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

refactor(ui): UI Navigation Refactoring #5076

Merged

Conversation

jjoyce0510
Copy link
Collaborator

@jjoyce0510 jjoyce0510 commented Jun 2, 2022

Summary

In this PR, 2 important things are happening:

  1. We are renaming the 'Manage' drop down in the Nav bar to 'Govern' - this is to more accurately represent its contents going forward.
  2. We are moving both Access Policy management and Users & Groups management into the Settings panel. Don't worry - we redirect from the previous urls to the new settings locations (your urls will continue to work). This is to clear space and declutter the navigation bar, which we know is overflowing for smaller screens.

We are super excited about these changes, they will clear the runway for more functionality and improve the organization of the app significantly. See the changes in the screenshots below.

Changes

  1. Remove users & groups, policies from top nav bar.
  2. Add users & groups, policies inside an "Access" section of the settings panel. only visible if the correct platform privileges are maintained.
  3. Add copy URL action buttons to Container, Domain, Tag pages.
  4. Other minor styling improvements.

Screenshots
Screen Shot 2022-06-02 at 4 44 54 PM

Screen Shot 2022-06-02 at 4 44 44 PM

Screen Shot 2022-06-02 at 4 44 39 PM

Screen Shot 2022-06-02 at 4 44 34 PM

Screen Shot 2022-06-02 at 10 33 55 AM
Screen Shot 2022-06-02 at 10 34 13 AM

Checklist

  • The PR conforms to DataHub's Contributing Guideline (particularly Commit Message Format)
  • Links to related issues (if applicable)
  • Tests for the changes have been added/updated (if applicable)
  • Docs related to the changes have been added/updated (if applicable). If a new feature has been added a Usage Guide has been added for the same.
  • For any breaking change/potential downtime/deprecation/big changes an entry has been made in Updating DataHub

@github-actions
Copy link

github-actions bot commented Jun 2, 2022

Unit Test Results (build & test)

333 tests  ±0   333 ✔️ ±0   2m 45s ⏱️ -2s
  77 suites ±0       0 💤 ±0 
  77 files   ±0       0 ±0 

Results for commit 5f0f880. ± Comparison against base commit b2d957d.

♻️ This comment has been updated with latest results.

Copy link
Collaborator

@chriscollins3456 chriscollins3456 left a comment

Choose a reason for hiding this comment

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

Looks good!

}}
/>
</Tooltip>
<CopyUrn urn={urn} isActive={copiedUrn} onClick={() => setCopiedUrn(true)} />
Copy link
Collaborator

Choose a reason for hiding this comment

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

love it

@jjoyce0510 jjoyce0510 changed the title refactor(ui): Misc minor styling improvements. refactor(ui): UI Navigation Refactoring Jun 2, 2022
Copy link
Contributor

@dexter-mh-lee dexter-mh-lee left a comment

Choose a reason for hiding this comment

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

Looks amazing!!!

@shirshanka shirshanka merged commit 4da3d13 into datahub-project:master Jun 3, 2022
Copy link
Collaborator

@chriscollins3456 chriscollins3456 left a comment

Choose a reason for hiding this comment

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

Looks good! two small nits but it looks like this PR was merged while I was re-reviewing

/**
* URL Paths for each settings page.
*/
const PATHS = [
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe SETTINGS_PATHS? was confused below when you were mapping over "PATHS"

<Redirect to={`${pathname}${pathname.endsWith('/') ? '' : '/'}${DEFAULT_PATH.path}`} />
</Route>
{PATHS.map((p) => (
<Route path={`${path}/${p.path.replace('/', '')}`} render={() => p.content} key={p.path} />
Copy link
Collaborator

Choose a reason for hiding this comment

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

why do you have this replace logic here if there's no "/" in any of the paths above?

@mydq
Copy link

mydq commented Jun 6, 2022

When will this pr be merged and released?

maggiehays pushed a commit to maggiehays/datahub that referenced this pull request Aug 1, 2022
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.

5 participants