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
[jext] create a title bar with adaptive theming, title, and logo #2545
Conversation
ec4fefc
to
e40065e
Compare
Last up is tests and then I'm ready for review. |
header { | ||
display: flex; | ||
justify-content: flex-start; | ||
background-color: var(--theme-menu-bg, rgb(250, 250, 250)); |
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.
👌
display: flex; | ||
justify-content: flex-start; | ||
background-color: var(--theme-menu-bg, rgb(250, 250, 250)); | ||
padding: 10px 16px; |
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.
We have some spacing
css variables at this point. Don't feel like you need to update this now, but it might help with consistency in the future.
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.
Ah, cool. What should I be using 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.
https://github.com/nteract/nteract/blob/master/packages/styles/src/config.json#L145
I think var(--nt-spacing-m)
and then var(--nt-spacing-l)
might work? There are always going to be places where you just need to push pixels a little bit, but I think we should strive for as much consistency here as possible
const mapDispatchToProps = dispatch => ({ | ||
titleChanged: () => { | ||
// TODO: Once the content refs PR is finished use the ref to change | ||
// the filename, noting that the URL path should also 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.
👌
@@ -237,3 +237,8 @@ export const modalType = createSelector( | |||
(state: AppState) => state.modals.modalType, | |||
identity | |||
); | |||
|
|||
export const currentTheme: (state: *) => string = createSelector( |
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.
Nice! This all looks great!
Howdy! I'm 🔓🤖! In order to keep information timely (based on the most recent release), we want all activity to be added to either new issues or open issues and PRs. In service to that goal, I, the lock bot close inactive closed issues when they haven't had activity in 120 days. Feel free to open a new issue for related bugs and link to relevant comments from this thread. |
Closes #2459.
I'm going to hold myself back from trying to tweak CSS too much here, open to some fresh style contributions.