Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
App Header React migration #4245
App Header React migration #4245
Changes from 10 commits
f2f8db8
4bd3461
91b3ab9
e55125f
a3aa95b
8b5ea0c
ad2f19c
962c631
b0c2894
536d0da
9115e9a
2d25cd3
fc5a9f5
66fc55a
7b3fbf7
4508544
a64dfe9
1c2904a
426e0c1
bedfa07
a50af02
8f6d290
35e1f5d
2f749d2
853eca3
97bb08e
c76a2c9
ef77bef
275c660
eec5e3e
dca65c7
fdd2f05
acce095
bf2306e
82834b7
816da58
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Although implicit (and it's not a blocker for this) I would prefer naming those
DesktopHeader
andMobileHeader
(or Navbar 🙂).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.
+1 for this 🙂
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.
It annoys me that this depends on a backend config and it doesn't fit our Redash-preview case, for example... I wonder if the above works for Multi-org 🤔
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 actually works for multi-org!
Learned sth new today :)
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.
Considering the Help Trigger has a Tooltip on its own, wdyt about adding a Tooltip here as well? So will be a "standard" Icon Button behavior
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.
Done. Also converted it to
<Button>
so it has the cool click animation.