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

Feature/new omnibar #60

Closed
wants to merge 4 commits into from
Closed

Feature/new omnibar #60

wants to merge 4 commits into from

Conversation

jamesmadson
Copy link
Collaborator

This is a PR to revise the omnibar and add in the proper styles/html for Accounts.

  • Add instructions for implementing Accounts (Lucian)
  • Review with Kirby and Lucian

@github-actions
Copy link

Azure Static Web Apps: Your stage site is ready! Visit it here: https://delightful-wave-007c8ea0f-60.eastus2.azurestaticapps.net

Copy link
Contributor

@sawiggins sawiggins left a comment

Choose a reason for hiding this comment

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

Nice and yay for SJC accounts!

I don't love the exaggerated spacing in the nav bar as it looks unintentional until you hover and then I get it but don't know that it worth it (it also gets really tall on mobile). The pill shapes inside the rounded white boxes are also too many rounded boxes for me. I'd consider another hover option and bringing in the space between the list elements.

Have we checked the contrast for the research domain text, the tiny gray text, and the light blue external link icons? It all seems really light.

A few ideas offhand. Perhaps, for the research domains, we could fill in the pill shape to 100% and use white text. The text under the support column needs additional emphasis on hover – perhaps an underline or matching the apps column. Or since we have so much space horizontally for the omnibar, maybe the hover is out to one side (though I guess that could get tricky with the app column).

<span class="logo"></span>
</a>
<a class="sjc-title" href="https://stjude.cloud">St. Jude Cloud</a>
<span class="portal-title">
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd update "portal" to "app" where we see it going forward to keep our language consistent.

/>
<span>
Genomics Platform
<div class="app-subtext">Data &amp; Tools</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

If we add the app's subtitles, the one for Genomics Platform should be updated to "Sequencing Data and Analysis" rather than using our carry-over language from before the redesign.

-->
<span>
JM
<!-- This is the user's initials, but we could add profile images later -->
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we ready to set this up to be populated via accounts rather than hardcoded (even if it is initials)?

@jamesmadson
Copy link
Collaborator Author

@sawiggins Thanks for the review, great points. I'll make sure to address them in the official PR. I set up this PR as a Draft, I still have work to finalize and review with Kirby.

@claymcleod claymcleod deleted the branch master August 7, 2021 18:44
@claymcleod claymcleod closed this Aug 7, 2021
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.

None yet

3 participants