Navbar UI updates #1674
Navbar UI updates #1674
Conversation
thatbudakguy
force-pushed
the
signin-ui
branch
from
c797feb to
06ac4d8
Compare
| white-space: normal; | ||
| visibility: hidden; | ||
|
|
||
| + small { |
There was a problem hiding this comment.
What is the purpose of this? What content is in the small that we want hidden? Can we just put the hidden style on that element in the template so it's easier to understand the connection?
There was a problem hiding this comment.
Spotlight by default inserts the exhibit title and some other content, but we don't want to show it because DLME has only one exhibit and the title is rendered elsewhere. I think this rendering happens in spotlight, so we'd need to override an entire template to do that. Part of the point of this PR was reducing the amount of things we override.
There was a problem hiding this comment.
Added a comment to note the above
| @@ -253,28 +262,34 @@ body { | |||
| } | |||
|
|
|||
| .topbar { | |||
| position: absolute; | |||
| width: 100vw; | |||
There was a problem hiding this comment.
Why is these necessary? Topbar is already full width.
There was a problem hiding this comment.
The existing styles don't have an effect once its position is changed to absolute, which is necessary to overlay it on the masthead.
There was a problem hiding this comment.
Why are we changing to absolute? I'm really struggling to understand that purpose of these changes.
There was a problem hiding this comment.
We overrode several spotlight templates in order to construct an HTML setup where the navbar was inside the masthead, in order to implement the current design. This PR gets rid of all the overrides in favor of creating that appearance via a small amount of CSS instead, so we don't diverge from Spotlight so much. It also means that we get to keep the default navbar, so it automatically appears on pages that do not have a masthead, like login pages, which is part of the ticket.
There was a problem hiding this comment.
Specifically, we use absolute so that the navbar can be positioned over the top of the masthead, which is its following sibling.
thatbudakguy
force-pushed
the
signin-ui
branch
2 times, most recently
from
708e077 to
433b47e
Compare
This ensures that the header and navbar are still displayed on pages where the masthead is not (like auth flow pages), and removes some places where we overrode Spotlight. See #1372
thatbudakguy
force-pushed
the
signin-ui
branch
from
433b47e to
aeb2bce
Compare
app/assets/stylesheets/dlme.scss
Outdated
| } | ||
|
|
||
| // Turn off transparency for the topbar on pages where there's no masthead |
There was a problem hiding this comment.
Can you add "(e.g. the pages provided by Devise)"
app/assets/stylesheets/dlme.scss
Outdated
| } | ||
| } | ||
|
|
||
| .masthead .topbar .navbar-nav { | ||
| .topbar .navbar-nav { |
| .masthead .site-title { | ||
| text-align: center; | ||
| .masthead .site-title-container { | ||
| display: none; |
There was a problem hiding this comment.
Can you add a comma here with the reason we're not displaying it?
app/assets/stylesheets/dlme.scss
Outdated
| @@ -275,6 +281,11 @@ body { | |||
| text-decoration: underline; | |||
| } | |||
| } | |||
|
|
|||
| // hide the "sign in" link | |||
| &[href="/users/sign_in"] { | |||
There was a problem hiding this comment.
Can we remove this and just not render the link https://github.com/sul-dlss/dlme/blob/main/app/views/shared/_user_util_links.html.erb#L41-L45
There was a problem hiding this comment.
Yeah, good call. made that change
Ref #1372. First commit makes it so that the navbar still appears on auth pages.