-
Notifications
You must be signed in to change notification settings - Fork 3
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
Prominent references to DOC #539
Conversation
Pull Request Test Coverage Report for Build 2238113413
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, left a couple clarifying questions
@@ -72,6 +71,15 @@ const CoBranding = styled.div` | |||
width: 100%; | |||
`; | |||
|
|||
const DocButton = styled.a` | |||
background: ${colors.caption}; | |||
color: ${colors.footerBackground} !important; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious what's the reason for using !important
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In Wrapper
styles above in that component there is an a
selector, that change the color of links. And this button is the link, so we need to override color property.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's interesting, I hadn't worked much in CSS before and looking into this, it's my understanding that this is because the wrapper's .Wrapper a
css selector is more specific than the DocButton's .DocButton
selector, so the Wrapper's a
styles override DocButton's styles, is that right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, exactly
@@ -89,7 +91,7 @@ const SiteNavigation: React.FC<ShareButtonProps> = ({ openShareModal }) => { | |||
params: { tenantId: tenant.id }, | |||
})} | |||
> | |||
{tenant.name} | |||
{isTablet ? tenant.name : tenant.docName} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we want to hide the docName if our width is tablet size? What about abbreviating the docName like "Pennsylvania DOC"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we want to hide the docName if our width is tablet size? What about abbreviating the docName like "Pennsylvania DOC"?
cc @hobuobi
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we trying to avoid two lines of text like this?
This not happens, when the App initially renders in smaller screen sizes. I assume this is the problem with useBreakpoint
hook and its behavior on dynamic screen size change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what you mean by this not happens?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean the two lines case not happens, when we load page on small screens.
I saw this happening only when resizing window in DevTools.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ping @hobuobi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All good to me after the last change!
Description of the change
Added prominent DOC references
Type of change
Related issues
Closes #525
Checklists
Development
These boxes should be checked by the submitter prior to merging:
Code review
These boxes should be checked by reviewers prior to merging: