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

[WEB-3869] Remove the Meganav Account Panel dropdown #408

Merged
merged 7 commits into from
Jul 18, 2024
Merged

Conversation

aralovelace
Copy link
Contributor

@aralovelace aralovelace commented Jul 10, 2024

Jira Ticket Link / Motivation

WEB-3869

Summary of changes

remove the content under the Account dropdown.
Created MeganavItemsSignedIn in the storybook for testing preview for this component

How do you manually test this?

  • Go to storybook page
  • Go to Core -> `MeganavItemsSignedIn
  • the preview of this component should be displayed

Screenshot:

image

Screenshot in the Voltaire local

image

Reviewer Tasks (optional)

Merge/Deploy Checklist

  • Written automated tests for implemented features/fixed bugs
  • Rebased and squashed commits
  • Commits have clear descriptions of their changes
  • Checked for any performance regressions

Frontend Checklist

  • No frontend changes in this PR
  • Added before/after screenshots for changes
  • Tested on different platforms/browsers with Browserstack
  • Compared with the initial design / our brand guidelines
  • Checked the code for accessibility issues (VoiceOver User Guide)?

skip 14.1.6 due to error I did last time when pre-releasing develooment
@aralovelace
Copy link
Contributor Author

hi @jamiehenson this is now ready for review. its works in the local and screenshot added in the description

Copy link
Member

@kennethkalmer kennethkalmer 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! One small request to make storybook experience a bit better


theme: {},

absUrl: () => "https://ably.com/search?",
Copy link
Member

Choose a reason for hiding this comment

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

I would make absUrl an "identity" function, ie have it return whatever is provided as the argument, because it took me a while to figure out why the button in storybook goes to this URL and not the one in account.links.dashboard.href

Copy link
Member

@jamiehenson jamiehenson left a comment

Choose a reason for hiding this comment

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

Thanks for providing a screenshot on how it looks in situ in Voltaire - I think with this in mind we should adjust the padding/margin between the account name (i.e. Free Account) and the search icon, so that it's the same size as the gap between the search icon and the Dashboard button

import MeganavItemsSignedIn from "./MeganavItemsSignedIn";

const meta = {
component: MeganavItemsSignedIn,
Copy link
Member

Choose a reason for hiding this comment

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

We should relocate this story to be a story within the existing Meganav.stories.tsx file, I think - this sits better when considering Storybook as both a development and a documentation resource.

Copy link
Member

@jamiehenson jamiehenson Jul 17, 2024

Choose a reason for hiding this comment

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

Also, a wrapper around the component that imposes a maximum width would help to present this component in a way which is more visually recognisable to the actual Meganav (the flex behaviour means that the account name is way over on the left, when it should be next to it - a wrapper with a constrained width would help here).

Take a look at some of the other stories within this repo for examples of how render: () => <>code here<> works in stories, that allows you to render the component whilst also modifying how it's presented.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jamiehenson I agree with your comment, I did some updates

  • I removed the MeganavItemsSignedIn.stories
  • updated Meganav.stories.tsx by adding MeganavSignedIn component so the Default component will always be not logged-in and the MeganavSignedIn will always be SignedIn version. I updated the HTTP request with some dummy default values since the API seems not working yet?

@aralovelace
Copy link
Contributor Author

Thanks for providing a screenshot on how it looks in situ in Voltaire - I think with this in mind we should adjust the padding/margin between the account name (i.e. Free Account) and the search icon, so that it's the same size as the gap between the search icon and the Dashboard button

Thanks for noticing this bit @jamiehenson I removed the margin right and it looks the same gap now -
image

Copy link
Member

@jamiehenson jamiehenson left a comment

Choose a reason for hiding this comment

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

Changes look great! 🙌 Only thing I would change is MeganavSignedin to SignedIn in the Meganav story so that it comes up as "Signed In" in the Storybook navigator - it's a bit cleaner.

Added a new SignedInStory
- Default will be not login by assigning the sessionData empty http handler
- Added a new  Meganav SignedIn story with signed in data by adding a dummy http response account signed in

adjusting the space between the account name and search icon
@aralovelace aralovelace merged commit f52a2a6 into main Jul 18, 2024
3 checks passed
@aralovelace aralovelace deleted the WEB-3869 branch July 18, 2024 13:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

3 participants