-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Dashboard design updates #29896
Dashboard design updates #29896
Conversation
Pinging @elastic/kibana-design |
Fantastic work. I'll do a more thorough review quickly since I know we're coming down to the wire. cc @elastic/kibana-app team to be aware we're working on this and it would be nice to have some eyes on it early next week. Because these changes can cause rendering changes in dashboards, our thought is to target this only at 7 only and consider it a (easily addressable) breaking change for existing dashboards. This means our own prebuilt dashboards in our data data tutorials. The "fix" will be to simply resize panels if they've shifted because of the new padding / title / font shifts. This likely was already happening due to the font changes in 7 anyway. I most cases this should be extremely minor. Also @asawariS, @alexfrancoeur and @AlonaNadler should check out these changes. My guess is that the read only mobile view is something worth broadcasting in some way as we ramp up our launch. Along with our new nav, more and more of Kibana will be available (but maybe not optimized from a payload perspective) for smaller screens. One thought there @cchaos, we might want to just hide the edit button in mobile using one of our wrappers or utility classes. |
💔 Build Failed |
💔 Build Failed |
70d9c70
to
7736d17
Compare
💚 Build Succeeded |
CI Passed, so no longer WIP. @snide I'd love to hide that edit button, but I think there are some implications with that and we'd have to do some hacky stuff with adding the label as an append to the classname of all menu items which isn't great because of any multi-word labels. |
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.
Minor comments below.
I did a more general scan of this visually tonight. I think it touches on all the stuff we jammed on and seems good to me. As an amendment to my previous comment I also think it likely will likely have very minimal effects on existing dashboards (because in almost all places you've made things smaller), at least when looking at all the sample ones, so that makes it much less scary.
The only thing I'm guessing will be controversial with others is the legend toggling. Personally, I think this is a much cleaner way to handle it and would always optimize towards the read view of things rather than the edit views (since it will likely be seen many more times than edited). But we should at least give some PM / app team feedback here before merging. I think it is less obvious in the visualization app itself, which a trade-off I think we should be comfortable with.
The other additions are all fantastic and this cleans up dashboards considerably.
Small IE issue I noticed, but otherwise IE looks fine. Checked all other browsers for ya and they were fine.
src/legacy/core_plugins/kibana/public/dashboard/grid/_dashboard_grid.scss
Outdated
Show resolved
Hide resolved
src/legacy/core_plugins/kibana/public/dashboard/panel/_dashboard_panel.scss
Outdated
Show resolved
Hide resolved
7736d17
to
8e50c88
Compare
💚 Build Succeeded |
💚 Build Succeeded |
@markov00 @emmacunningham Caroline changed the design of the legend toggle button a bit (see "4. Legend toggle" above). I think we should look on aligning design in elastic-charts for that or similar. Maybe worth syncing up on that with @cchaos |
Great work!! Some comments and feedback below. I didn't have a chance to read through the comments, so apologies for any duplicate info.
|
@alexfrancoeur two tone will get fixed with elastic/eui#1513 when it makes its way over (with a kb pr as well) |
Thanks @alexfrancoeur !
Yeah, nothing I did should have interfered with that. Most likely still a symptom of the dependency.
Yeah, what @snide said. I've got some follow-ups that will fix all those. |
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 have a couple of minor things noted in the code, and one accessibility issue:
- You can no longer see which Options button is focused in Edit mode. Earlier that was visible since only the focused button had a background color, now every button has, and there is no visual distinction for the focused button anymore. We need a visual difference for the focused button.
Also I want to point out a weird behavior in mobile mode. As soon as you click on Edit
all panels are jumping to their original positions and you get the squeezed panel view. As soon as you leave edit mode everythings back to "responsive" design. I understand why it's behaving like that, since we can't properly reorder any panels in responsive mode, since their "grid position" are just not there, but I want to point this out to @AlonaNadler and @alexfrancoeur again, that this behavior might also confuse users, and just want to raise awareness for it, even though I am not really vetoing against it.
src/legacy/core_plugins/kibana/public/dashboard/panel/panel_error.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/reporting/export_types/common/layouts/preserve_layout.css
Outdated
Show resolved
Hide resolved
Responding to @timroes comment, can we simply remove edit mode when viewing through mobile? Would that be an even more confusing experience? Would love to hear @AlonaNadler thoughts here as well |
The best I can do right now is hide the |
..And used a different selector for editing mode to fix bug of `.dshPanel--editing` needing a hard refresh to update.
- background fix - dark mode highlight color fix - euiScrollbars - euiSpacer
- No edit button in mobile - No markdown comment for panel error - i18n id addition - Remove euiHeader sharing fix
69bdba9
to
0840302
Compare
💔 Build Failed |
Hopefully flaky... Jenkins, test this |
💚 Build Succeeded |
Great improvements here, really exciting stuff, congrats!! |
@AlonaNadler Thanks for taking a look! I think that makes sense, though out of scope of this PR as this mainly addresses visual updates. I would recommend creating an issue (if one doesn't already exist). |
This PR attempts to clean up the visuals (including spacing) of the dashboard (and some vis's)
1. States
a. Loading
I replaced the
Loading...
text with EUI's loading bar chart. However, I could only do this at the dashboard panel level and couldn't find a way to add it to the empty.visChart
component which loads shows empty for a long while.b. Visual changes to edit mode
Panel's now have a permanent dashed border and hovering over areas that are used for moving and resizing (react-layout stuff), they highlight in yellow.
c. Switching between edit and view modes fixed
There is a "bug" where the individual dashboard panel's wouldn't get the edit/view state update until a hard refresh. The panel's visual state was based on this "bug" so when you switched between from edit to view mode, it still looked like you were in edit mode. I was able to fix this by using the layout class
.dshLayout--editing
instead as this did get updated correctly.2. Panel titles
a. Panels no longer take up space if they don't exist.
It used to be that the space alloted for panel titles would still be present even if the title wasn't. Now, the content correctly shifts instead.
b. Titles also now use a EUI title style which are darker and bolder.
3. Errors
I've tried to consolidate the variety of error messages across the different vis types (excluding vega cuz that's just built weird). They should all be a small icon with a small subdued text description. With tag cloud, I moved the message into a tooltip and just display a warning icon.
4. Legend toggle
a. One location
The legend toggle was a tricky one. As a design team, we decided that we'd like to keep the legend toggle in one location and change the icon to represent 'legend' not a direction (arrow). This way when a user is searching for the legend, the toggle is always in the same place, even though the legend may appear in different locations.
b. Toggle states
We decided that the legend toggle just added noise to the dashboard when not interacting with it. So for view mode of the dashboard, we've hidden the toggle until hovering of the panel (similar to the way we handle the panel options menu).
Also, the button has an on/off state – currently just a background color in the on state.
This also means that the toggle has been moved in Visualize, but it's always visible.
Note: TSVB's legend wasn't touched. Though it could probably just use an icon update.
5. Bonus: Mobile
Our dashboard are completely unusable/unviewable on small device screens. I've implemented just a vertically stacked view on these small devices by overriding react-grid's layout styles and just display: blocking them.
Editing is completely broken when viewing in this stacked manner, so it only does this in view mode.
6. Other
a. I also had some adjustments to do in Visualize regarding the new font sizes.
b. I adjusted the controls vis a bit to allow for better wrapping of components and rearranged the button order and styles
c. I noticed that one of the error types was spitting out Markdown, so I tried to use markdown-it to parse it, but I think it needs some extra sanitizing or something because it just spits out the html as a string. Needs some help here
Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.For maintainers