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 support for teleporting areas of interest content outside RAMP #498

Merged
merged 1 commit into from
Nov 12, 2024

Conversation

yileifeng
Copy link
Member

@yileifeng yileifeng commented Nov 7, 2024

Related Item(s)

For https://github.com/ramp4-pcar4/tcei-tmx-cwa-storylines/issues/74, #499

Changes

  • [FEATURE] option to teleport areas of interest items outside of RAMP instance (to the right)
  • [FIX] change tooltip positioning for horizontal ToC items to be placed at bottom of container

Notes

Here is what the TCEI summaries map would look like:

image

Note: I kept the same styling as the old point of interests that was overlayed on top of RAMP instance but open to any styling changes now that it is side-by-side.

Testing

Take a look at the interactive map slide on main demo page.


This change is Reviewable

Copy link

github-actions bot commented Nov 7, 2024

Copy link
Member

@spencerwahl spencerwahl left a comment

Choose a reason for hiding this comment

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

Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @yileifeng)

Copy link
Member

@RyanCoulsonCA RyanCoulsonCA left a comment

Choose a reason for hiding this comment

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

Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @yileifeng)

Copy link
Member

@RyanCoulsonCA RyanCoulsonCA left a comment

Choose a reason for hiding this comment

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

Actually just randomly found a bug when testing this again on mobile resolution.

On mobile the points of interest are layered back on top of the map, but it makes scrolling the page very difficult. The non-teleported implementation of this puts the point of interest layer overtop of the map and makes the width 100% so the map can't be interacted with. If we still want the map to be interacted with on mobile then we need to find another way to fix this. Maybe the built-in scroll guard fixture works?

scrolling.gif

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @yileifeng)

Copy link
Member

@james-rae james-rae left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @yileifeng)


StoryRampSchema.json line 275 at r1 (raw file):

                    "description": "A title that is displayed centered above this map."
                },
                "teleportAOI": {

I'm fairly out of my depth on this one, but rando thought is if you make this a string with an enum / domain-of-values, it will future-proof it to adding different teleport targets, like to the left of the map, or some of those other fancy storylines content layouts.
'off' or '' can be default.
Can call the current "on" mode 'right', or just 'on' and you can alias that to 'right' later if you need to expand.

Anyway just a thought, do what ya wanna!

@yileifeng yileifeng force-pushed the interactive-map-teleport branch from 355d8e7 to 549ebab Compare November 7, 2024 20:17
Copy link
Member Author

@yileifeng yileifeng left a comment

Choose a reason for hiding this comment

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

Moved the areas of interest content outside RAMP container on mobile resolutions below the map, similar to the original StoryMap.

Reviewable status: 0 of 4 files reviewed, all discussions resolved (waiting on @RyanCoulsonCA and @spencerwahl)


StoryRampSchema.json line 275 at r1 (raw file):

Previously, james-rae (James Rae) wrote…

I'm fairly out of my depth on this one, but rando thought is if you make this a string with an enum / domain-of-values, it will future-proof it to adding different teleport targets, like to the left of the map, or some of those other fancy storylines content layouts.
'off' or '' can be default.
Can call the current "on" mode 'right', or just 'on' and you can alias that to 'right' later if you need to expand.

Anyway just a thought, do what ya wanna!

Changed alongside the "teleportGrid" config option for consistency

@yileifeng yileifeng force-pushed the interactive-map-teleport branch from 549ebab to 3b20f8b Compare November 7, 2024 20:24
@james-rae
Copy link
Member

StoryRampSchema.json line 275 at r1 (raw file):

Previously, yileifeng (Yi Lei Feng) wrote…

Changed alongside the "teleportGrid" config option for consistency

If you really wanna go ham, you can add something like this to your schema (use whatever string values you had in mind).

"enum": ["", "right"]

@yileifeng yileifeng force-pushed the interactive-map-teleport branch from 3b20f8b to 8fa8215 Compare November 8, 2024 13:35
Copy link
Member Author

@yileifeng yileifeng left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 4 files reviewed, all discussions resolved (waiting on @RyanCoulsonCA and @spencerwahl)


StoryRampSchema.json line 275 at r1 (raw file):

Previously, james-rae (James Rae) wrote…

If you really wanna go ham, you can add something like this to your schema (use whatever string values you had in mind).

"enum": ["", "right"]

Donethanks

Copy link
Member

@spencerwahl spencerwahl left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 4 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @yileifeng)

@yileifeng yileifeng force-pushed the interactive-map-teleport branch from 8fa8215 to bf9202b Compare November 8, 2024 14:46
Copy link
Member Author

@yileifeng yileifeng left a comment

Choose a reason for hiding this comment

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

Added a change to adjust tooltip positioning for items in horizontal ToC to be placed at bottom: #499

Reviewable status: 4 of 5 files reviewed, all discussions resolved (waiting on @james-rae, @RyanCoulsonCA, and @spencerwahl)

Copy link
Member

@spencerwahl spencerwahl left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @yileifeng)

Copy link
Member

@RyanCoulsonCA RyanCoulsonCA left a comment

Choose a reason for hiding this comment

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

Looks good! We might want to consider adding support for scroll guard on the map at some point, but we don't have to do it in this PR

Reviewed 3 of 4 files at r2, 1 of 1 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @yileifeng)

@yileifeng yileifeng merged commit 8ac62f0 into ramp4-pcar4:main Nov 12, 2024
3 checks passed
@yileifeng yileifeng deleted the interactive-map-teleport branch November 12, 2024 16:46
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.

4 participants