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 interactive-image panel #525

Merged
merged 1 commit into from
Jan 3, 2025

Conversation

spencerwahl
Copy link
Member

@spencerwahl spencerwahl commented Dec 27, 2024

Changes

  • [FEATURE] Adds the interactive-image panel, to fix issues with the hacky implementation in a project and to make it reusable.

Notes

A large portion of the panel file is taken from dynamic panels. The change looks large but it shouldn't be too crazy to look through.

Testing

Steps:

  1. Scroll down to the new slide
  2. Hover over the left zone, see nothing change
  3. Click the zone, see the right panel change like a dynamic panel
  4. Hover the right zone, see a new image
  5. Click and see the image stay and the right panel update

Please test in mobile, keyboard etc. Things worked for me but who knows what happens when things become a demo.


This change is Reviewable

Copy link

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 and works great! Tested on Chrome & Firefox with keyboard and on mobile. The only issue I found is that on mobile the text overlaps with the image as you scroll down:

textoverlap.gif

Reviewed 5 of 5 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @spencerwahl)


src/components/panels/interactive-image-panel.vue line 127 at r1 (raw file):

onMounted(() => {
    if (props.slideIdx && props.slideIdx > 2) {

This should probably take into account the lazyLoad flag, which disables lazy loading if set to false. You should just need to add in props.lazyLoad && here, and then add the following to the props variable up top (the prop should already be passed in to all panels so nothing more is needed):

lazyLoad: {
        type: Boolean
}

src/components/panels/interactive-image-panel.vue line 186 at r1 (raw file):

const zoneMouseEnter = (zone: InteractiveImageZone): void => {
    console.log(zone);

Stray console.log

Copy link
Member Author

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

Huh I must have accidentally removed the fix for that. Donethanks.

Reviewable status: 4 of 5 files reviewed, 2 unresolved discussions (waiting on @RyanCoulsonCA)


src/components/panels/interactive-image-panel.vue line 127 at r1 (raw file):

Previously, RyanCoulsonCA (Ryan Coulson) wrote…

This should probably take into account the lazyLoad flag, which disables lazy loading if set to false. You should just need to add in props.lazyLoad && here, and then add the following to the props variable up top (the prop should already be passed in to all panels so nothing more is needed):

lazyLoad: {
        type: Boolean
}

done


src/components/panels/interactive-image-panel.vue line 186 at r1 (raw file):

Previously, RyanCoulsonCA (Ryan Coulson) wrote…

Stray console.log

done

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.

I'm still seeing the same behavior shown in the GIF

Reviewable status: 4 of 5 files reviewed, 2 unresolved discussions (waiting on @RyanCoulsonCA)

Copy link
Member Author

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

Ok it should actually be fixed this time, sick me put the class on the wrong element 🏆

Reviewable status: 4 of 5 files reviewed, 2 unresolved discussions (waiting on @RyanCoulsonCA)

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

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 4 of 5 files at r1, 1 of 1 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @spencerwahl)

@yileifeng yileifeng merged commit 434903f into ramp4-pcar4:main Jan 3, 2025
3 checks passed
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.

3 participants