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

chore: Remove footer and move links to status bar MAASENG-2058 #5193

Merged
merged 5 commits into from
Oct 13, 2023

Conversation

ndv99
Copy link
Contributor

@ndv99 ndv99 commented Oct 12, 2023

Done

  • Moved footer links to status bar
  • Removed footer
  • Moved relevant tests from footer suite to status bar suite
  • Changed status bar colour to #F7F7F7

QA steps

  • Log in
  • Ensure the footer is no longer there
  • Ensure the "Local documentation" and "Legal information" links are present in the status bar, and they work

Fixes

Fixes:

Screenshots

Before

image

After

image

Notes

The "Give feedback" link may not be visible locally, since this only appears in production (and if "Enable analytics" is true). You can test this by negating line 110 in src/app/base/components/StatusBar/StatusBar.tsx

@webteam-app
Copy link

Demo starting at https://maas-ui-5193.demos.haus

@@ -208,3 +210,50 @@ it("displays last image sync timestamp for a rack or region+rack controller", ()
`Last image sync: ${controller.last_image_sync}`
);
});

it("displays the feedback link when analytics enabled and not in development environment", () => {
process.env = { ...originalEnv, NODE_ENV: "production" };
Copy link
Contributor

Choose a reason for hiding this comment

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

Do these changes to process.env persist beyond these tests? I would suggest reverting it after each test in this suite is run to prevent it from causing problems elsewhere

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good point, thanks for spotting this. Will follow up with a commit to reset this after each test

Copy link
Contributor

@Jay-Topher Jay-Topher 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, I left a few comments

Local documentation
</a>
</li>
<li className="p-inline-list__item">
Copy link
Contributor

Choose a reason for hiding this comment

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

Should these links open in place or in a new tab? Please advise @dgtlntv

Copy link
Contributor Author

Choose a reason for hiding this comment

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

+1. I copied the original implementation directly, which opens them in the same tab, but some UX guidance would be really helpful here

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, I'll go ahead and approve this pending UX review

Copy link
Member

Choose a reason for hiding this comment

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

Good point! I think opening in a new tab would be better!

@dgtlntv
Copy link
Member

dgtlntv commented Oct 12, 2023

I realised just now that, because the status bar is light blue, the contrast between the status bar and the link is too small so it doesn't pass WCAG AA for small text. We might need to adjust either of the colours.

@dgtlntv
Copy link
Member

dgtlntv commented Oct 12, 2023

And as mentioned in the chat I think we should try to make the "Give feedback" button work again while we are at it.

@ndv99
Copy link
Contributor Author

ndv99 commented Oct 12, 2023

I realised just now that, because the status bar is light blue, the contrast between the status bar and the link is too small so it doesn't pass WCAG AA for small text. We might need to adjust either of the colours.

IMO I think the status bar colour is the better/easier one to change. Link colour is defined within vanilla AFAIK, so changing it in the footer would mean less consistency with Vanilla.

The old design used to have a dark footer with light text, which may be better. And in any case, the status bar is the only place we see this colour being used in the UI, so changing it won't affect consistency with anything else.

@dgtlntv
Copy link
Member

dgtlntv commented Oct 12, 2023

Agreed, changing the link colour wouldn't be a good idea. The color for the status bar in the Vanilla example is #F7F7F7 so I would propose to change it to that. I think it would also improve the visual hierarchy of MAAS, as the status bar currently doesn't contain any important information the light blue seems too dominant.

</a>
</li>
<li className="p-inline-list__item">
<a
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason why you couldn't use Link from react components?

We actually have an ExternalLink in maas-site-manager that has the behaviour you discussed (e.g. opening in a new tab built-in). That makes it a good candidate for addition to the library. I'll look into this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good point, we should add that. Only reason I didn't use it here was that I was direct;y copying the original implementation in the footer. Will update to use this.

@ndv99 ndv99 merged commit 407743f into canonical:main Oct 13, 2023
5 checks passed
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