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

Updated nav #4548

Merged
merged 25 commits into from
Jan 11, 2024
Merged

Updated nav #4548

merged 25 commits into from
Jan 11, 2024

Conversation

allisonking
Copy link
Contributor

@allisonking allisonking commented Jan 5, 2024

Closes https://ethyca.atlassian.net/browse/PROD-1517

Description Of Changes

New nav ✨

Code Changes

  • Edit _app.tsx such that the nav and header render on every page (except login) and fixed
  • Edit main content so that we always scroll within a container (essentially, what FixedLayout was)
  • Consolidate all Layout's into one Layout (which is mostly FixedLayout but with the nav and header moved out into _app.tsx. It needs to be in _app.tsx otherwise you'll see flickering in the nav when you go between pages)
  • Update unit tests and cypress tests
  • Update smoke test (still need to do the same in fidesplus)
  • Remove breadcrumbs
  • Remove no longer used nav components

Steps to Confirm

  • Click around in the preview environment and make sure everything still looks okay!

Pre-Merge Checklist

Copy link

vercel bot commented Jan 5, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
fides-plus-nightly ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 11, 2024 4:46pm

Copy link

cypress bot commented Jan 5, 2024

Passing run #5956 ↗︎

0 4 0 0 Flakiness 0
⚠️ You've recorded test results over your free plan limit.
Upgrade your plan to view test results.

Details:

Merge 20126f5 into 475d1e7...
Project: fides Commit: 8f06fe5a9e ℹ️
Status: Passed Duration: 00:34 💡
Started: Jan 11, 2024 4:54 PM Ended: Jan 11, 2024 4:55 PM

Review all test suite changes for PR #4548 ↗︎

@simonkeane33
Copy link

simonkeane33 commented Jan 10, 2024

@allisonking just to check, I'm still seeing the breadcrumb, is that to be expected?
I'm using this link : https://fides-plus-nightly-4yjbvgz4o-ethyca.vercel.app/consent/reporting

@allisonking
Copy link
Contributor Author

@simonkeane33
Copy link

All looks good to me @allisonking apart from the margin around the back button. I have it set to 8px above and 12px below. Otherwise looks great.

html: {
height: "100%",
},
"#__next": {
Copy link
Contributor

Choose a reason for hiding this comment

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

clever! This div has caused issues for me in the past.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah yes, I spent much time chasing down all the heights that needed to be 100%...

Copy link
Contributor

@TheAndrewJackson TheAndrewJackson left a comment

Choose a reason for hiding this comment

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

Great refactor! I just left a small question about whether the back button on the add system(s) pages is going to the correct spot. Other than that everything looked great!

@allisonking allisonking merged commit 7efe458 into main Jan 11, 2024
13 checks passed
@allisonking allisonking deleted the aking/prod-1517/updated-nav branch January 11, 2024 17:21
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