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

improve header loading process #2427

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Conversation

doug-s-nava
Copy link
Collaborator

@doug-s-nava doug-s-nava commented Oct 9, 2024

Summary

Fixes (unticketed)

Time to review: 15 mins

Changes proposed

updates to allow header nav items to remain consistent between page navigations.

Context for reviewers

because the list of nav items was being instantiated as empty on first render, when loading the layout on navigation, there would be a flicker of the nav, waiting for the items to be created on the second render. With this change, the nav will be rendered by default without any feature flagged items, and feature flagged items will appear on second render. There are implementations that could prevent flickering, but we'd have to implement feature flags or translation a little differently, I think.

Test steps

  1. on main, with search feature flag off, navigate around different pages on the app
  2. VERIFY: on each navigation the header nav links flicker on each navigation
  3. on this branch, with search feature flag off, navigate around different pages on the app
  4. VERIFY: nav links remain in place while navigating through the app
  5. on this branch, with search feature flag on, navigate around different pages on the app
  6. VERIFY: search link appears in nav on second render
  7. turn off search feature at /dev/feature-flags
  8. VERIFY: search link still appears in nav until page is refreshed. This is an existing issue that this change does not deal with, but we should consider refactoring the useFeatureFlags hook to respond to changes in feature flags without refreshing

Additional information

Screenshots, GIF demos, code examples or output to help show the changes working as expected.

@doug-s-nava doug-s-nava marked this pull request as ready for review October 10, 2024 00:02
const navLinksFromFlags = featureFlaggedNavLinkConfigs.reduce(
(acc, link) => {
const { flag = "" } = link;
console.log("####", featureFlags[flag]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
console.log("####", featureFlags[flag]);

Looks like this got left in here.

@acouch
Copy link
Collaborator

acouch commented Oct 10, 2024

Looks good so far. I still see flickering in the navigation, even if the feature flag code is removed. I would think that Next could be more efficient about not needing to re-render navigation. Not going to look further into it myself for the time being.

@doug-s-nava
Copy link
Collaborator Author

Pushing pause on this for now, may need to refactor the useFeatureFlags hook to get this working properly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants