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

UIKit Nav Cleanup #1102

Merged
merged 16 commits into from
Jan 7, 2023
Merged

UIKit Nav Cleanup #1102

merged 16 commits into from
Jan 7, 2023

Conversation

sghoweri
Copy link
Contributor

@sghoweri sghoweri commented Nov 24, 2019

Long overdue front-end cleanup of the Nav component to be more maintainable, more easily updated in the future and more performant. Not 100% where I want to be with this ideally (maybe 80%+) but definitely puts us in a much better spot than before.

At a high level, this includes one new feature, but most of this is just under the hood refactoring.

New Feature (Fix?)

  • Only allow one top level nav link open at a time while on larger screens / nav displays as a dropdown menu

Refactor

  • Updates us from using Preact v8 to Preact v10 so we can fully take advantage of Fragments, etc
  • Breaks the Nav up into smaller, more manageable sub-components
  • Significantly reduces the amount of UI logic required (especially with handling the active Nav item)
  • Simplifies the CSS to deduplicate and streamline everything; sets us up for eventually allowing end users to officially be able to customize the UI via CSS Custom Properties

I still need to finish up doing cross browser testing on this but early testing seems very promising.

Side Note: I mistakenly branched this off of #1034 so I'd recommend we merge that down first to avoid merge conflicts + reduce any potential rework!

@coveralls
Copy link

coveralls commented Nov 24, 2019

Coverage Status

Coverage remained the same at 76.959% when pulling a2e22ee on feature/uikit-nav-refactor into a5a1265 on dev.

@bmuenzenmeyer
Copy link
Member

@sghoweri are you using this to make progress against #1103 or can it be independently merged?

@sghoweri
Copy link
Contributor Author

Independent — actually started this before #1103!

@sghoweri
Copy link
Contributor Author

Update: there's at least two small CSS fixes that'll be needed here before this is good to go in IE 11 + Firefox, FYI!

@stale
Copy link

stale bot commented Mar 11, 2020

It's hard to keep track of everything. This issue has been automatically marked as stale because it has not had recent activity, neither from the team nor the community. It will be closed if no further activity occurs. Please consider adding additional info, volunteering to contribute a fix for this issue, or making a further case that this is important to you, the team, and the project as a whole. Thanks!

@bmuenzenmeyer
Copy link
Member

@sghoweri just checking in on this - I havent reviewed in quite a while, citing your above comment

@JosefBredereck
Copy link
Contributor

Update: there's at least two small CSS fixes that'll be needed here before this is good to go in IE 11 + Firefox, FYI!

Do you need a review for this topic or is it not finished at the moment?

@stale
Copy link

stale bot commented Jul 27, 2020

It's hard to keep track of everything. This issue has been automatically marked as stale because it has not had recent activity, neither from the team nor the community. It will be closed if no further activity occurs. Please consider adding additional info, volunteering to contribute a fix for this issue, or making a further case that this is important to you, the team, and the project as a whole. Thanks!

@stale
Copy link

stale bot commented Dec 19, 2020

It's hard to keep track of everything. This issue has been automatically marked as stale because it has not had recent activity, neither from the team nor the community. It will be closed if no further activity occurs. Please consider adding additional info, volunteering to contribute a fix for this issue, or making a further case that this is important to you, the team, and the project as a whole. Thanks!

@JosefBredereck JosefBredereck merged commit f95f4f9 into dev Jan 7, 2023
@JosefBredereck JosefBredereck deleted the feature/uikit-nav-refactor branch January 7, 2023 18:47
antonia-rose pushed a commit to quelltexterin/nemo-uikit-workshop that referenced this pull request Apr 12, 2023
* feat: upgrade to Preact v10

* refactor: break apart Nav into smaller more maintainable pieces; significantly reduce UI logic required under the hood

* refactor: minor tweaks to Pattern State UI

* fix: fix the drag offset of the drawer resizer

* chore: update yarn.lock

* refactor: rename root Nav component files

* pattern-lab#1102 After merge fixes

* fix: yarn lock after merge

* fix: review findings in css and js

* fix: remove unused code and reimplement utility func

Co-authored-by: Brian Muenzenmeyer <brian.muenzenmeyer@gmail.com>
Co-authored-by: Josef Bredreck <slime.games@outlook.de>
Co-authored-by: Josef Bredreck <13408112+JosefBredereck@users.noreply.github.com>
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.

5 participants