-
Notifications
You must be signed in to change notification settings - Fork 4
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
Add new realm to map #675
Conversation
✅ Deploy Preview for taho-development ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this 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
There was a problem hiding this 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.
src/shared/constants/realms.ts
Outdated
realm4, | ||
realm7, | ||
realm9, | ||
realm19, | ||
realm22, | ||
] | ||
|
||
export const ADDITIONAL_REALMS_MAP_DATA: RealmMapData[] = [realm16] | ||
|
||
export const REALMS_MAP_DATA = process.env.SHOW_ADDITIONAL_REALMS |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will merge it tomorrow to not bloat the batch for QA before the XP drop |
</Fragment> | ||
))} | ||
</defs> | ||
{pathData.paths.map((_, index) => ( |
There was a problem hiding this comment.
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.
Resolves #637
What has been done:
useMultiRef
hookMake sure to add to
.env
to see the new realm (think it is quite universal for future additions):SHOW_ADDITIONAL_REALMS="true"