-
Notifications
You must be signed in to change notification settings - Fork 24
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
Guidance Update Feature #3502
Guidance Update Feature #3502
Conversation
👷 Deploy request for keen-mayer-a86c8b accepted.
|
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.
Nice work! I'm still working on reviewing but here are some minor comments so far
Looks like there is a conflict with the |
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.
Another round of comments
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.
Last few comments!
Can you also run prettier on your files that you changed? (option+shift+f)
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.
Just need to fix the yarn.lock conflict, otherwise looks good!
hey quick heads up: i noticed a bug where everything runs smoothly if you access the updated components via the components/templates page, but it only flashes the updates (inline notifications & side navigation indicators) when you enter the guidance through the search bar. i'm pushing up a version with the most recent comments resolved and quick edit to the route names, but after that push i'm going to be working on that problem. |
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.
A few tiny comments, but I think the PR looks good otherwise!
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.
I think we are good to merge, nice work!
I'm running this locally right now and getting a variety of linting errors. Working through fixing them before we merge. |
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.
Looks great!
Deploy Preview
What does this PR do?
Added an update feature that tracks changes to the guidances. It uses the Github API to read the pull requests closed onto the site, then displays the information via inline notifications and tags. It also uses visitation tracking for a personalized experience, this makes it so once the user has seen the update on the guidance page, it won't show again.
Where should the reviewer start?
They should start in the app.js file in the pages folder. This has the fetching information and calls the other components with that information. It would then be beneficial to explore the functions called/added in app.js from there.
What testing has been done on this PR?
I've tested that the feature against older PRs (making sure they wrote over the old ones), tested it in incognito with no access to local storage, and different cases with last-visted tokens.
In addition to the feature you are implementing, have you checked the following:
General UX Checks
Accessibility Checks
Code Quality Checks
How should this be manually tested?
To test visitation tracking or to see how information is being stored, I would check the local storage in the browser's developer tools. A user who is entirely new to the site will have neither "update-history" or any tokens ending in "-last-visited," but when launching, it will create the "update-history."
Any background context you want to provide?
To run the visitation tracking correctly (in dev mode) I had to turn the React StrictMode to false when writing it. I turned it back to true before this push/pull, but to run it with proper logic it should be toggled to false.
What are the relevant issues?
Issue #3405
Screenshots (if appropriate)
Should this PR be mentioned in Design System updates?
Yes
Is this change backwards compatible or is it a breaking change?
Note about pageUpdateReady:
In app.js there is a declared state of pageUpdateReady. This is used to accommodate for the rendering behavior when entering a guidance page. Here is the context we need it for: