Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add new realm to map #675

Merged
merged 13 commits into from
Nov 17, 2023
Merged

Add new realm to map #675

merged 13 commits into from
Nov 17, 2023

Conversation

xpaczka
Copy link
Contributor

@xpaczka xpaczka commented Nov 10, 2023

Resolves #637

What has been done:

  • Added new realm to the island
  • Added support for multi part path cutouts
  • Added useMultiRef hook
  • Removed not needed clouds

Make sure to add to .env to see the new realm (think it is quite universal for future additions):

  • SHOW_ADDITIONAL_REALMS="true"

@xpaczka xpaczka self-assigned this Nov 10, 2023
Copy link

netlify bot commented Nov 10, 2023

Deploy Preview for taho-development ready!

Name Link
🔨 Latest commit 4b107d2
🔍 Latest deploy log https://app.netlify.com/sites/taho-development/deploys/65572f34245c95000859d267
😎 Deploy Preview https://deploy-preview-675--taho-development.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@xpaczka xpaczka marked this pull request as ready for review November 13, 2023 12:28
Copy link
Contributor

@jagodarybacka jagodarybacka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great 🔥

I think we can:

  • allow to open the realm modal, take care of the UI there
  • check the pin placement both on the map and on the realm modal header

Also if we will just hide the new realm behind the feature flag then we can merge it

src/ui/Island/Realm.tsx Outdated Show resolved Hide resolved
src/ui/Island/Realm.tsx Outdated Show resolved Hide resolved
src/shared/utils/island.ts Outdated Show resolved Hide resolved
src/shared/utils/island.ts Outdated Show resolved Hide resolved
@jagodarybacka
Copy link
Contributor

I just noticed - on the realm modal only the "main land" of the realm is visible and the mini island are missing

image

Copy link
Contributor

@jagodarybacka jagodarybacka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One small thing and we are good to go. If we will decide to make other part of the map active I think it will be easier to do it in the next PR.

realm4,
realm7,
realm9,
realm19,
realm22,
]

export const ADDITIONAL_REALMS_MAP_DATA: RealmMapData[] = [realm16]

export const REALMS_MAP_DATA = process.env.SHOW_ADDITIONAL_REALMS
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it must be process.env.SHOW_ADDITIONAL_REALMS === "true" becuase env vars are strings so SHOW_ADDITIONAL_REALMS="false"will break it

jagodarybacka
jagodarybacka previously approved these changes Nov 16, 2023
Copy link
Contributor

@jagodarybacka jagodarybacka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@jagodarybacka
Copy link
Contributor

Will merge it tomorrow to not bloat the batch for QA before the XP drop

</Fragment>
))}
</defs>
{pathData.paths.map((_, index) => (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's put it in one iteration of array mapping.

@jagodarybacka jagodarybacka merged commit ca1566b into main Nov 17, 2023
3 checks passed
@jagodarybacka jagodarybacka deleted the new-realm branch November 17, 2023 09:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add new Realm (tentative)
3 participants