-
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
View/edit mode for dashboards #9508
Conversation
d6941f8
to
c58984b
Compare
c58984b
to
abefdcc
Compare
9f52531
to
8140f2b
Compare
Looks like the build failed on this one |
jenkins test this |
0d59125
to
d080e3e
Compare
d080e3e
to
a8e2fd3
Compare
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.
This is awesome, I really like it.
My only concern is that while editing a dashboard, the user can click the 'Stop Editing' button without saving their changes. I could see a user hit 'Stop Editing' as opposed to 'Save' and assuming the changes that they made were saved, because it kinda looks like it.
|
||
function getViewConfig(modeChange) { | ||
return createTopNavExecuteConfig( | ||
'Stop Editing', |
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.
Small nitpick, the 'key' for this config doesn't match the others.
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.
because of the case? I did that on purpose because I want the second word capitalized. The first word gets auto capitalized but the second doesn't. I could do stop editing
and it'll show up as Stop editing
but I didn't like that. I could do stop Editing
but that's weird too. I suppose I could also find where these words are capitalized and capitalize every word there, but that might affect other tabs. I'll revert to stop editing
to be consistent here and we can decide on the casing later, and change it for all or nothing.
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.
Sorry, please disregard this entirely. I made this comment a while ago, and it's no longer valid.
Hmm, that's a really good observation. I see two ways to fix this.
I think in most cases #2 would be a better UX, but #1 is safer. Thoughts? Any input on this @cjcenizal? |
Part of me wonders whether having a single 'Save/Cancel Editing' button that changes based on the appState.isDirty would be sufficient. |
On second thought, maybe always having a 'Save' and 'Cancel Editing' button would be better than my previous suggestion, and disable the Save button before there are changes. I liked your idea about showing the user when there are unsaved changes as well, but I dunno if my idea about disabling the 'Save' button would be clear enough. |
28891a1
to
2535116
Compare
How about a toaster message (not red) when they switch to view mode with unsaved changes? Without better test coverage (both unit and integration) around the dirty flag, we probably should not be disabling the save capability on it. We've had issues already with it not being entirely reliable with reporting in x-pack. |
and implement new angular style suggestions from Joe
- Move safe_confirm into modals folder - make data-test-subj camelCase. - get rid of $timeout call, doesn’t seem necessary anymore, now that window.confirm is not being used.
and fix some bugs NOTE: code is a mess. Once UX is nailed down I’ll clean it up.
c5265f7
to
17a130c
Compare
Things required before this PR can move forward:
Maybe required:
Figure out later:
|
Still TODO: - Add clickable breadcrumbs - Remove New and Open top nav options
improve tests
- Add new landing page empty text (merge) - Make view mode stateful
Implementation of #9431