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

Spinner and Page Navigation issues #3543

Merged
merged 2 commits into from
Oct 2, 2023

Conversation

MVarshini
Copy link
Contributor

@MVarshini MVarshini commented Sep 1, 2023

PBENCH-1263

Spinner and Page Navigation issues

  • On clicking Header logo, it will re-direct to home page
  • All Runs on side menu will show Coming soon text
  • Spinner is center aligned
  • Breadcrumb on Results page removed
  • Ordered Route constants

@MVarshini MVarshini self-assigned this Sep 1, 2023
@MVarshini MVarshini added the Dashboard Of and relating to the Dashboard GUI label Sep 1, 2023
@MVarshini MVarshini added this to the v0.73 milestone Sep 1, 2023
dbutenhof
dbutenhof previously approved these changes Sep 1, 2023
Copy link
Member

@dbutenhof dbutenhof left a comment

Choose a reason for hiding this comment

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

Approving, but with a few minor issues ...

dashboard/src/App.js Show resolved Hide resolved
Copy link
Member

@webbnh webbnh left a comment

Choose a reason for hiding this comment

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

I think that we should be using the symbolic references instead of literals in sideMenuOptions.js.

dashboard/src/App.js Show resolved Hide resolved
dashboard/src/modules/components/HeaderComponent/index.jsx Outdated Show resolved Hide resolved
Copy link
Member

@webbnh webbnh left a comment

Choose a reason for hiding this comment

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

The changes look good; I just have one question.

Comment on lines 58 to 59
export const menuOptionsNonLoggedIn = [
{ key: "dashboard", link: "/", name: "Dashboard" },
Copy link
Member

Choose a reason for hiding this comment

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

In light of the change to HeaderComponent/index.jsx line 145, should the link field here should be changed to "/" + APP_ROUTES.HOME? (I think the answer is "no", but I don't know why. 😞)

@MVarshini MVarshini force-pushed the PBENCH-1263 branch 2 times, most recently from a471c63 to 95868cf Compare September 7, 2023 07:29
Spinner and Page Navigation issues
@dbutenhof
Copy link
Member

Since your rebase didn't clear the approvals, I'm tempted to merge this ... but I have a feeling that, with the CI ubi9 container problems we're seeing that will break the main build, and I'm not sure that's a good idea. 😦

@dbutenhof dbutenhof merged commit 277df9b into distributed-system-analysis:main Oct 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Dashboard Of and relating to the Dashboard GUI
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants