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

UX: Re-use left sub-menu for Settings and License page on admin dashboard #16427

Merged
merged 2 commits into from
Feb 16, 2023

Conversation

Devansu-Yadav
Copy link
Contributor

Description

  • Updates the Settings and License page layout to re-use the left sub-menu for better UX.

Related Issue(s)

Fixes #16399

How to test

Release Notes

Update the Settings and License page layouts for the admin dashboard

Documentation

Build Options:

  • /werft with-github-actions
    Experimental feature to run the build with GitHub Actions (and not in Werft).
  • leeway-no-cache
    leeway-target=components:all
  • /werft no-test
    Run Leeway with --dont-test
Publish Options
  • /werft publish-to-npm
  • /werft publish-to-jb-marketplace
Installer Options
  • with-ee-license
  • with-slow-database
  • with-dedicated-emulation
  • with-ws-manager-mk2
  • workspace-feature-flags
    Add desired feature flags to the end of the line above, space separated

Preview Environment Options:

  • /werft with-local-preview
    If enabled this will build install/preview
  • /werft with-preview
  • /werft with-large-vm
  • /werft with-gce-vm
    If enabled this will create the environment on GCE infra
  • /werft with-integration-tests=all
    Valid options are all, workspace, webapp, ide, jetbrains, vscode, ssh

@Devansu-Yadav Devansu-Yadav requested a review from a team February 16, 2023 07:28
@werft-gitpod-dev-com
Copy link

annotations in the pull request changed, but user is not allowed to start a job

@Devansu-Yadav
Copy link
Contributor Author

@gtsiolis Do we need to make the Settings and License pages on the admin dashboard mobile responsive?

@gtsiolis
Copy link
Contributor

gtsiolis commented Feb 16, 2023

Do we need to make the Settings and License pages on the admin dashboard mobile responsive?

@Devansu-Yadav No, it's ok to leave these as they are for now for the following reasons:

  1. The changes in this PR don't need to increase the scope further.
  2. Our focus will shift away from the admin dashboard.
  3. The license page will probably be dropped, see relevant comment.
  4. The General, previously known as Settings page should be already quite responsive by relying on the existing layout.
  5. Responsive design has been in our radar but not a priority so far because of the main targe audience. I'm planning to discuss with @selfcontained next week more about areas we can improve around navigation, etc. in Epic: Improve dashboard responsive layout #4050. Please forward or open any issues that seem related to that epic. 🍊

/werft run with-preview-true

👍 started the job as gitpod-build-update-settings-layout-fork.0
(with .werft/ from main)

@gtsiolis
Copy link
Contributor

gtsiolis commented Feb 16, 2023

/werft run with-preview=true

👍 started the job as gitpod-build-update-settings-layout-fork.1
(with .werft/ from main)

@Devansu-Yadav
Copy link
Contributor Author

Devansu-Yadav commented Feb 16, 2023

  1. The General, previously known as Settings page should be already quite responsive by relying on the existing layout.

Yes, it is quite responsive already, except for the Tabs component below the header, which is not that responsive due to the no of tabs present on the admin dashboard.

  1. Responsive design has been in our radar but not a priority so far because of the main target audience. I'm planning to discuss with @selfcontained next week more about areas we can improve around navigation, etc. in Epic: Improve dashboard responsive layout #4050. Please forward or open any issues that seem related to that epic. 🍊

@gtsiolis Sure, in fact, I'd love to help out with this issue in case there haven't been any changes addressing the responsive layout for the dashboard 😄

Copy link
Contributor

@gtsiolis gtsiolis left a comment

Choose a reason for hiding this comment

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

UX LGTM! 🌮 🌮

Left one minor comment. If that's not trivial to resolve, let's merge this and open a follow-up issue or PR about it. 🏓

},
{
title: "License",
link: ["/admin/license"],
Copy link
Contributor

Choose a reason for hiding this comment

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

issue: The Admin menu becomes inactive when navigating to the license page. Could we keep it active?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed it! Was just a one-liner fix that was needed 😆
Actually, I completely missed out that updating the getAdminTabs function to remove the License page route might cause such an issue, as this function is being used to set the active state on the admin menu 😅.

@gtsiolis
Copy link
Contributor

gtsiolis commented Feb 16, 2023

/werft run with-preview=true

👍 started the job as gitpod-build-update-settings-layout-fork.2
(with .werft/ from main)

Copy link
Contributor

@gtsiolis gtsiolis left a comment

Choose a reason for hiding this comment

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

UX LGTM, @Devansu-Yadav! 🌮 🌮

Comment on lines +71 to +79
alternatives: [
...getAdminTabs().reduce(
(prevEntry, currEntry) =>
currEntry.alternatives
? [...prevEntry, ...currEntry.alternatives, currEntry.link]
: [...prevEntry, currEntry.link],
[] as string[],
),
],
Copy link
Contributor

Choose a reason for hiding this comment

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

thought: Not sure about this piece of code here, but UX looks good. I'll loop in @gitpod-io/engineering-webapp for visibility in case this needs a revert or a different approach.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, no problem!
Basically, this code populates all the top-level admin dashboard routes obtained through the getAdminTabs function, which are then passed to the isSelected function to set the active state for the admin menu on these top-level routes. Hope this makes it a bit clear 😅

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for clarifying, @Devansu-Yadav! 🙇

@roboquat roboquat merged commit 4692a05 into gitpod-io:main Feb 16, 2023
@Devansu-Yadav
Copy link
Contributor Author

@gtsiolis Based on your comment in #16408, I was probably looking to pick up one of the following issues - #10499, #3836 or #3930. Should I proceed with these issues, or could you suggest any other issues related to the dashboard component?

@Devansu-Yadav Devansu-Yadav deleted the update-settings-layout branch February 17, 2023 11:48
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.

Update settings layout for the admin dashboard
3 participants