-
Notifications
You must be signed in to change notification settings - Fork 1
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
STAN-288: Create Associates microservice for StoryBlocks #709
Conversation
src/components/page-types/associatesDirectoryPage/Directory/Associate.jsx
Outdated
Show resolved
Hide resolved
@moisesnarvaez I left a few preliminary comments for you When this is ready for review please tag myself (@sherakama) and (@yvonnetangsu) as reviewers and mark this as ready for review. |
Thanks for the early review, I implemented all the suggestions. The PR is now ready for review. |
@moisesnarvaez I think you're missing the masthead (The top bar with logo/nav/search) on the directory page. Can you add it in? |
src/components/page-types/associatesDirectoryPage/Directory/AssociateList.jsx
Outdated
Show resolved
Hide resolved
@sherakama As discussed, I'm asking for @rebeccahongsf 's help to review this instead of myself, since she worked on the original widget and is more familiar with the functionality 😄 Thank you @rebeccahongsf in advance 😂 |
src/components/page-types/associatesDirectoryPage/legacyDirectory/LegacyDirectory.jsx
Outdated
Show resolved
Hide resolved
@moisesnarvaez sorry for the slow review but I finally got through it. This is looking and working great. I added a few comments to this PR. Overall, just some cleanup and perhaps a little error handling. Thank you for creating the API layer for fetching contentful data.
|
@sherakama thanks for your review! I implemented the 1-3 requested changes. About the 4. Can you please guide me? I added the component based on the other page, what else do I need to do to display the header? |
@moisesnarvaez I found a small bug (?) that's pretty strange. If I tab over to eg, "C-D" and hit the I'm on Mac Ventura Chrome 116. |
Trigger build
@yvonnetangsu Can you please elaborate on what the expected behavior is? On other pages (like this Pull Request page for example), it is the same:
|
That's a great catch @yvonnetangsu ! I was able to reproduce the issue on Google Chrome as well. The keyboard accessibility differs from the prod Associates site. On production, the user is able to switch between tabs with the arrow keys. 🤔 @moisesnarvaez would you be able to update the keyboard accessibility interaction to match production? |
@rebeccahongsf I see, thanks for clarifying, working on 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.
Thank you @moisesnarvaez for your work on this! I had a few requests regarding the user experience:
-
Update Heading to h1 and add styles to match other h1s across the site
-
Add spacing to the top and bottom of the main content
-
Move the Directory component into the main container
-
Adjust tab styling for mobile
-
Would you be able to add the
Back to top
link to each section so that users can be brought back to the top of the widget? You can view it on production by scrolling towards the bottom of section A (or end of the page)
Other than those items, everything else is looking good to me. I'll do another pass once these changes have been implemented. Please do not hesitate to tag me if additional information is needed or if you have any questions. Thanks again! 😃
src/components/page-types/associatesDirectoryPage/associatesDirectoryPage.js
Outdated
Show resolved
Hide resolved
src/components/page-types/associatesDirectoryPage/Directory/Tabs.jsx
Outdated
Show resolved
Hide resolved
src/components/page-types/associatesDirectoryPage/associatesDirectoryPage.js
Outdated
Show resolved
Hide resolved
@rebeccahongsf I implemented all of the requested changes, along with other minor refactoring and CSS adjustments. |
3b522d0
to
7022db9
Compare
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.
Thank you for the super quick changes, @moisesnarvaez! 🥳
A few more design and accessibility tweaks and I think we should be all set!
-
Update Container wrapping the Directory component to use
width="full"
-
Adjust Associates wrapper from
<div>
to<li>
for accessibility -
Adjust Associates List wrapper from
<div>
to<ul>
for accessibility -
Add styles
su-p-0 su-list-none
to Results list to remove additional spacing and possible list styles
src/components/page-types/associatesDirectoryPage/associatesDirectory.js
Outdated
Show resolved
Hide resolved
src/components/page-types/associatesDirectoryPage/Directory/Associate.jsx
Outdated
Show resolved
Hide resolved
src/components/page-types/associatesDirectoryPage/Directory/AssociateList.jsx
Outdated
Show resolved
Hide resolved
src/components/page-types/associatesDirectoryPage/Directory/Results.jsx
Outdated
Show resolved
Hide resolved
@rebeccahongsf Thanks for your kind reviews =). I just pushed the adjustments. One question, once this PR is merged, who will take care of the required Storyblock changes in Prod? |
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.
Thanks again @moisesnarvaez for implementing the requested changes. The associates page looks great to me! 🎉
As for your question:
One question, once this PR is merged, who will take care of the required Storyblock changes in Prod?
Either @sherakama or I could move the storyblok blocks to production and setup the page. I defer to Shea for final confirmation though 😄
@moisesnarvaez I'll add you to the production instance if you can confirm that the changes are all new items and we aren't having to change or migrate anything existing. Once you've completed the changes in storyblok we will merge the code. |
That's correct @sherakama, the items are new. I have documented them in the description of this PR, in the section called: |
I have added you to two new Storyblok spaces for the Prod and Dev instances. Please make the appropriate changes to each Storyblok environment. We will first roll the code through dev to validate deployment went well, and then we will push the code to production. Let me know when you have completed the changes. |
@sherakama thanks, all configuration has been replicated. Dev: https://app.storyblok.com/#/me/spaces/118608/stories/0/0/378510601 PD: I'm assuming the parent folder for the Content page is "Stanford Associates". |
Great. Let's start with Dev. |
Dev looks good in the Storyblok editor. I'll publish it and validate tomorrow that the build works and then we can schedule a release. |
@moisesnarvaez & @William-Misiaszek Is there a desired release date for production on this functionality? |
@sherakama Ideally we release it October 11th in Production (but not published in StoryBlok). Cc: @moisesnarvaez |
Ok thanks. I'll put a release event on the calendar for the 11th. |
READY FOR REVIEW
Summary
Steps followed to complete the requirement requested:
Review By (Date)
Review Tasks
Setup tasks and/or behavior to test
2.1. List associates in alphabetical order, showing the years on each one.
2.2. Tab navigation
2.3. Search by name
2.4. Filter only new members
Associated Issues and/or People
Steps to be Done in the SotyBlocks Prod Account
Blocks
associatesDirectory
:associatesDirectroryPage
:associatesDirectory
associatesDirectory
Content Page (suggested)
Type: Story
Name: Directory
Parent folder: "Stanford Associates"
Content type: Associates Directory Page
Prestet: none