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

ensure map is focused before teleported grid #540

Merged
merged 1 commit into from
Feb 7, 2025

Conversation

RyanCoulsonCA
Copy link
Member

@RyanCoulsonCA RyanCoulsonCA commented Feb 5, 2025

Related Item(s)

https://github.com/ramp4-pcar4/tcei-tmx-cwa-storylines/issues/190

Changes

  • Modified the order of the map component and the teleport container in the map panel. This should ensure that the map is focused before the teleported panel is.

Notes

While this now ensures that the map is focused prior to the grid when the grid is teleported to the right of the map, we will now have the same issue pop up if the grid is teleported to the left of the map.

It doesn't seem like there's a great solution for this. Adding tabindex conditionally makes the focus jump all over the place. I could potentially duplicate the div with the grid-teleport class and swap the elements depending on whether the grid is teleported to the left or right, but that adds a bunch of unnecessary duplication to the code.

Testing

Steps:

  1. Open the demo page.
  2. Tab through the 000 product until you get to the first map slide (should the third slide down from the top)
  3. Ensure that the map is focused before the teleported grid.

This change is Reviewable

@RyanCoulsonCA RyanCoulsonCA added the PR: Frontend PR that primarily involves frontend changes. UI experts and CSS Wizards are asked to review. label Feb 5, 2025
Copy link

github-actions bot commented Feb 5, 2025

Your demo site is ready! 🚀 Visit it here: https://ramp4-pcar4.github.io/story-ramp/fix-190/#/en/00000000-0000-0000-0000-000000000000

Copy link
Member

@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.

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

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.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @RyanCoulsonCA)


src/components/panels/map-panel.vue line 26 at r1 (raw file):

                class="storylines-grid-container flex-1 min-w-0 min-h-0 ramp-styles"
                :class="{
                    'sm:order-1 order-2': config.teleportGrid === 'left',

The simplest solution for both cases is probably to duplicate this and use a v-if instead of doing the order trickery.

Doesn't need to be done now if this is needed ASAP

Copy link
Member Author

@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.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @spencerwahl)


src/components/panels/map-panel.vue line 26 at r1 (raw file):

Previously, spencerwahl (Spencer Wahl) wrote…

The simplest solution for both cases is probably to duplicate this and use a v-if instead of doing the order trickery.

Doesn't need to be done now if this is needed ASAP

Yep, I agree. I'll have it done this afternoon 👍

Copy link
Member Author

@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.

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @spencerwahl and @yileifeng)


src/components/panels/map-panel.vue line 26 at r1 (raw file):

Previously, RyanCoulsonCA (Ryan Coulson) wrote…

Yep, I agree. I'll have it done this afternoon 👍

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.

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @RyanCoulsonCA and @yileifeng)


src/components/panels/map-panel.vue line 26 at r1 (raw file):

Previously, RyanCoulsonCA (Ryan Coulson) wrote…

Donethanks

I think you can just remove all of the order classes?

Copy link
Member Author

@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.

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @spencerwahl and @yileifeng)


src/components/panels/map-panel.vue line 26 at r1 (raw file):

Previously, spencerwahl (Spencer Wahl) wrote…

I think you can just remove all of the order classes?

I've removed the order-2 class from the right grid element, but it looks like we need to keep it on the other two elements so that the map still appears above the grid when in mobile mode.

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 r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @RyanCoulsonCA)


src/components/panels/map-panel.vue line 26 at r1 (raw file):

Previously, RyanCoulsonCA (Ryan Coulson) wrote…

I've removed the order-2 class from the right grid element, but it looks like we need to keep it on the other two elements so that the map still appears above the grid when in mobile mode.

lol yep, I forgot about mobile 👍

Copy link
Member

@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.

Reviewed all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @RyanCoulsonCA)

@yileifeng yileifeng merged commit f40189e into ramp4-pcar4:main Feb 7, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: Frontend PR that primarily involves frontend changes. UI experts and CSS Wizards are asked to review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants