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

DS-850 | Annotated image component #343

Merged
merged 43 commits into from
Oct 14, 2024
Merged

Conversation

yvonnetangsu
Copy link
Member

@yvonnetangsu yvonnetangsu commented Sep 6, 2024

READY FOR REVIEW

Summary

  • Add Annotated Image component

Review By (Date)

  • Retro

Criticality

  • 5

Review Tasks

Setup tasks and/or behavior to test

  1. Go to Momentum/Test/Yvonne Test Annotated Image
  2. Select PR DS-850 | Annotated image component #343 from the visual editor
  3. Play with all the variants on the page and test all responsive
  4. Quick check for accessibility (I've tested with Voice Over and it seems ok)

Associated Issues and/or People

Copy link

netlify bot commented Sep 6, 2024

Deploy Preview for giving-campaign ready!

Name Link
🔨 Latest commit 2ccacf4
🔍 Latest deploy log https://app.netlify.com/sites/giving-campaign/deploys/6709bf7b5fa17e0008fca57a
😎 Deploy Preview https://deploy-preview-343--giving-campaign.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.

…_annotated-image

* feature/DS-911_update-dependencies:
  update madr
  Update autoprefixer, postcss and react-loading-skeleton
  Update framer motion and heroicons
  Update headlessui
  Update @storyblok/react
  Update typescript and types
  Update React, netlify-cli
  Update next and netlify next plugin first

# Conflicts:
#	package-lock.json
#	package.json
* dev:
  3.0.2
  HOTFIX: API Throttling. (#355)
  3.0.1
  NoJira: Use preview key in background function.
  3.0.0
  Remove signature check due to Storyblok silliness. (#353)
  DS-917: Deploy webhook (#351)
  DS-916: Split paths and Slug Prefix. (#350)
@sherakama
Copy link
Member

This is looking good.

Screenshot 2024-10-11 at 15 38 55
Screenshot 2024-10-11 at 15 39 21
Screenshot 2024-10-11 at 15 39 29

The sizes of the nodes feel nice on my phone and don't seem too crammed together. It will be good to document and let the content authors know that the points can't be too close together or they will overlap on mobile.

One thing on mobile I noticed was a weird resize. When I click on a button to open the modal, the content gets a gap of content on the right side. Perhaps something to do with flexbox and a new modal div in the dom? The site jumps back to the correct size when the modal is closed.

@sherakama
Copy link
Member

The content author experience in Storyblok is nice. I like your grouping, field names, and descriptions.

export type SbImageHotspotModalContentType = 'text-image' | 'fullwidth-image' | 'text' | 'component';
export type SbImageHotspotDescriptionSizeType = 'card' | 'default' | 'big';

export type SbImageHotspotType = {
Copy link
Member

Choose a reason for hiding this comment

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

One day we will generate these from Storyblok...

@yvonnetangsu
Copy link
Member Author

The sizes of the nodes feel nice on my phone and don't seem too crammed together. It will be good to document and let the content authors know that the points can't be too close together or they will overlap on mobile.

One thing on mobile I noticed was a weird resize. When I click on a button to open the modal, the content gets a gap of content on the right side. Perhaps something to do with flexbox and a new modal div in the dom? The site jumps back to the correct size when the modal is closed.

Hey thanks for looking at this @sherakama! I just pushed up another big commit. Need another one to move all my styles in one place then I think I'm done. I don't seem to see the weird resize on my iPhone - I wonder if it's an android thing. It can have something to do with scrollbars. Next time we meet let's do a sharing 😄

You're probably out soon so I'll likely push this to PROD after another check. Since this is a new feature, I think if you find anything retro next week I can fix it after 🤠

Happy Canadian Thanksgiving - enjoy some quality family time and pie!! 🦃 🥧

@sherakama
Copy link
Member

I don't seem to see the weird resize on my iPhone - I wonder if it's an android thing.

I was able to reproduce it with my browser and the iPhone 14 Pro option. Do you see the layout shift when you open/close the modal in Chrome Devtools?

@sherakama
Copy link
Member

You're probably out soon so I'll likely push this to PROD after another check

Sure, I don't see anything that would stop it. I didn't do a very comprehensive A11y review however. I was able to tab navigate and the focus traps/sets were working well as far as I could tell.

You should probably take a pass with voiceover once before going live tho.

@yvonnetangsu
Copy link
Member Author

I don't seem to see the weird resize on my iPhone - I wonder if it's an android thing.

I was able to reproduce it with my browser and the iPhone 14 Pro option. Do you see the layout shift when you open/close the modal in Chrome Devtools?

Oh I see what you mean. I did see it on DevTools for iPhone but it doesn't happen on the real iPhone

@yvonnetangsu
Copy link
Member Author

You're probably out soon so I'll likely push this to PROD after another check

Sure, I don't see anything that would stop it. I didn't do a very comprehensive A11y review however. I was able to tab navigate and the focus traps/sets were working well as far as I could tell.

You should probably take a pass with voiceover once before going live tho.

Thanks - will do! I'll probably send this over to SODA as well.

@yvonnetangsu yvonnetangsu marked this pull request as ready for review October 14, 2024 17:32
@yvonnetangsu
Copy link
Member Author

Going to push this live. Tested on VO and iPhone. This won't be published until Nov so I'm not too worried about making it live now. 🤠

@yvonnetangsu yvonnetangsu merged commit 0971294 into dev Oct 14, 2024
5 checks passed
@yvonnetangsu yvonnetangsu deleted the feature/DS-850_annotated-image branch October 14, 2024 18:00
This was referenced Oct 14, 2024
yvonnetangsu added a commit that referenced this pull request Oct 14, 2024
* DS-850 | Annotated image component (#343)

* initial commit

* Set up SbAnnotatedImage

* linter

* Hotspots proto

* Use modal instead of popover; hotspot button style

* Update headlessui

* Allow nested components

* Modal content update

* modal styles

* Handle hotspot animation

* Redo hotspot animation based on feedback

* text and image variant for the modal

* More modal adjustments

* More modal

* Center hotspots

* Responsive and make sure it works when placed in a > 1 col grid

* Fix issue with bounding width and moving hotspots

* Use container query for hotspot size instead of media query

* Make sure hotspot relative position doesn't shift due to caption

* description font size options

* Variant with image only

* Properly check for caption

* Update next and netlify next plugin first

* Update React, netlify-cli

* Update typescript and types

* Update @storyblok/react

* Update headlessui

* Update framer motion and heroicons

* Update autoprefixer, postcss and react-loading-skeleton

* update madr

* Add test only modal variant

* Add quote vertical alignment option like on Tour site; style text-only modal

* linter

* Make sure crop height and width are defined even if nothing is passed in aspect ratio for StoryImage

* further clean up styles

* Image only variant

* undo my mess up for min height

* More options and variants

* Final clean up with styles and image source sizes

* Better key for hotspot li; honor priority loading option; lazy load ankle banner image (#358)

---------

Co-authored-by: Stanford Webservices <[email protected]>
yvonnetangsu added a commit that referenced this pull request Oct 31, 2024
* dev: (22 commits)
  3.2.1
  DS-1016 | Sync tour changes and clean up; update packages (#365)
  DS-850 | Annotated image tweak (#363)
  Nojira | More robust URL processing; prevent empty redirect entries causing build error (#362)
  3.2.0
  NoJira: Force token through sbParams. (#360)
  DS-918: Storyblok Redirects. (#357)
  3.1.0
  Better key for hotspot li; honor priority loading option; lazy load ankle banner image (#358)
  DS-850 | Annotated image component (#343)
  3.0.2
  HOTFIX: API Throttling. (#355)
  3.0.1
  NoJira: Use preview key in background function.
  3.0.0
  Remove signature check due to Storyblok silliness. (#353)
  DS-917: Deploy webhook (#351)
  DS-916: Split paths and Slug Prefix. (#350)
  2.5.8
  DS-911 | Update dependencies (#348)
  ...

# Conflicts:
#	app/(storyblok)/[[...slug]]/not-found.tsx
#	app/not-found.tsx
#	tailwind/plugins/theme/gc-keyframes.js
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.

2 participants