Skip to content
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

Introduce Clone feature in view mode #10925

Merged
merged 8 commits into from May 19, 2017

Conversation

Projects
None yet
8 participants
@stacey-gammon
Copy link
Contributor

commented Mar 29, 2017

Summary: Introducing the ability to easily clone dashboards in view mode. Just click the new button in the top navigation, and enter in a new dashboard name.

screen shot 2017-03-29 at 10 05 26 am

Fixes #10621

cc @alexfrancoeur

@stacey-gammon stacey-gammon force-pushed the stacey-gammon:clone-dashboard-feature branch 3 times, most recently from 6f9a604 to b2b47b0 Mar 29, 2017

@stacey-gammon stacey-gammon requested a review from cjcenizal Mar 30, 2017

@stacey-gammon stacey-gammon requested a review from kobelb Mar 30, 2017

@stacey-gammon stacey-gammon force-pushed the stacey-gammon:clone-dashboard-feature branch from b2b47b0 to af95ec3 Apr 6, 2017

@stacey-gammon stacey-gammon requested review from archanid and removed request for kobelb and cjcenizal Apr 6, 2017

@stacey-gammon

This comment has been minimized.

Copy link
Contributor Author

commented Apr 6, 2017

@cjcenizal Removing you from review, but feel free to comment on functionality if you prefer. @archanid I heard you'd like to review more stuff, so tagging you, hope you don't mind! 🥇

@alexfrancoeur Likewise, feel free to check out the functionality and comment if you like. You seemed partial to the previous clone functionality that got taken away. :)

@alexfrancoeur

This comment has been minimized.

Copy link
Contributor

commented Apr 6, 2017

@stacey-gammon you are correct, I love the clone concept :-) Functionally it works perfectly ++

My initial reaction was that I'd like to rename it immediately. What are your thoughts around providing similar functionality that Save has? Naming, storing time with dashboard, etc. Or is that something you were thinking should stay specific to Save in edit mode?

@stacey-gammon

This comment has been minimized.

Copy link
Contributor Author

commented Apr 6, 2017

Yea, good point @alexfrancoeur - renaming is a natural step immediately before or after a clone command. But we want the save panel to eventually go away and be a "one click save", so I don't know if adding another similar panel will be good in the long run.

We just really need an easier way to rename the dashboard without having to save it. :(

@alexfrancoeur

This comment has been minimized.

Copy link
Contributor

commented Apr 6, 2017

I agree and am certainly all for one click saving and inline dashboard naming. While we don't have those features, should we keep the "saving" of a dashboard consistent?

@stacey-gammon

This comment has been minimized.

Copy link
Contributor Author

commented Apr 6, 2017

@snide @cjcenizal Any perspective from the design team here? One click clone vs the same experience for save, vs... something we haven't thought of?

@kobelb

This comment has been minimized.

Copy link
Contributor

commented Apr 7, 2017

@stacey-gammon from a technical perspective, this LGTM. Though, I'm holding off on approving it until we get some design input.

@kobelb kobelb self-requested a review Apr 7, 2017

@stacey-gammon

This comment has been minimized.

Copy link
Contributor Author

commented Apr 7, 2017

Chatted with the design team and we decided to go with a pop up modal that provides an input field where the name can be changed, along with cancel and confirm clone buttons.

Given that, I expect this will get pushed to 5.5.

cc @cjcenizal @snide

@stacey-gammon stacey-gammon added v5.5.0 and removed v5.4.0 labels Apr 7, 2017

@alexfrancoeur

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2017

Sounds good @stacey-gammon. No need to rush, users still can save as new under edit mode. This functionality will be much more important when we begin to introduce more granular access control in the future.

@cjcenizal @snide I don't want to increase the scope of this PR by any means but I have a question about the proposed approach. If we introduce modals here for saving confirmation, do we need to keep that consistent across the board? Saving a visualization, saving a dashboard, etc. Or is cloning unique in this situation?

@cjcenizal

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2017

@alexfrancoeur I don't think the answer is as clearcut as "All 'saving' actions require modals" or "Only cloning needs a modal." I think surfacing a modal for cloning is a strong UX, and is an improvement worth introducing even as a standalone change. As for the rest of the header, we know it needs an overall redesign, and on a more granular level we know we want to improve the saving UX. I'd say we can move forward with the cloning modal now, and figure out the other saving UX improvements later (which may or may not involve modals).

@alexfrancoeur

This comment has been minimized.

Copy link
Contributor

commented Apr 11, 2017

@cjcenizal I agree that the the modal is a better UX than what we have now. However, I also know how important consistency is for UX. My concern is that introducing different ways to save items is confusing to a user. This was part of the reason why we had to revert back to the original saving approach for edit mode. It was too different from visualize, saved searches, etc. If design does not believe this will be a poor user experience, I'm all for introducing the modal. Otherwise, I don't see why wouldn't just keep them the same.

@cjcenizal

This comment has been minimized.

Copy link
Contributor

commented Apr 12, 2017

@snide What are your thoughts on @alexfrancoeur's points about preserving UX consistency, above?

super(props);

this.state = {
newDashboardName: props.title

This comment has been minimized.

Copy link
@nreese

nreese May 16, 2017

Contributor

Do you want the default value to cause a name collision with the existing dashboard title? Maybe Clone should be appended to the title or something like that, or maybe the default value is blank?

This comment has been minimized.

Copy link
@stacey-gammon

stacey-gammon May 16, 2017

Author Contributor

Yea, I originally had it with 'Copy' appended, wasn't sure which version I liked better. I can bring that back.

const kbnUrl = $injector.get('kbnUrl');
const confirmModal = $injector.get('confirmModal');
const Private = $injector.get('Private');

This comment has been minimized.

Copy link
@nreese

nreese May 16, 2017

Contributor

Why the switch to $injector.get()?

This comment has been minimized.

Copy link
@stacey-gammon

stacey-gammon May 16, 2017

Author Contributor

@spalger's preference for injector over multiline argument lists. :)

This comment has been minimized.

Copy link
@spalger
className="kuiTextInput kuiTextInput--large"
value={ this.state.newDashboardName }
onChange={ this.onInputChange } />
</KuiModalBodyText>

This comment has been minimized.

Copy link
@nreese

nreese May 16, 2017

Contributor

Should the model also let the user update the dashboard description for the clone? I could see a user cloning a dashboard and wanting to describe what the clone is going to be used for. Might be nice to complete this action in a single screen.

This comment has been minimized.

Copy link
@stacey-gammon

stacey-gammon May 16, 2017

Author Contributor

Yea, I could see that being helpful. Mind if I make that a separate enhancement/PR though so it doesn't hold this PR up?

This comment has been minimized.

Copy link
@nreese

nreese May 16, 2017

Contributor

not at all

This comment has been minimized.

Copy link
@stacey-gammon

stacey-gammon May 16, 2017

Author Contributor

Filed: #11817

@stacey-gammon stacey-gammon force-pushed the stacey-gammon:clone-dashboard-feature branch from eb015c5 to 2337cd0 May 16, 2017

@nreese

nreese approved these changes May 16, 2017

@kobelb

kobelb approved these changes May 16, 2017

Copy link
Contributor

left a comment

Nice job, really excited to see us using React for this!!
Just a few minor comments, nothing mission/critical or worth blocking this PR.

type="primary"
data-test-subj="cloneConfirmButton"
onClick={ this.cloneDashboard }
ref={ (button) => { this.confirmButton = button; } }

This comment has been minimized.

Copy link
@kobelb

kobelb May 16, 2017

Contributor

It looks like you can use the autoFocus attribute to do the same without having to use the ref.

This comment has been minimized.

Copy link
@kobelb

kobelb May 16, 2017

Contributor

Also, do we want the confirm button to be focused when the modal pops up? It seems like we'd want the textbox with the name to be focused, and make the enter key for that textbox trigger the Save.

};

onKeyDown = (event) => {
if (event.keyCode === 27) {

This comment has been minimized.

Copy link
@kobelb

kobelb May 16, 2017

Contributor

Can we add a comment to denote what keyCode 27 is? I had to look it up to figure out what this was intended to do.

I really like this behavior, and wonder whether we should see if the design-team would like to introduce it for all modals.

This comment has been minimized.

Copy link
@stacey-gammon

stacey-gammon May 16, 2017

Author Contributor

We have it for all the confirm modals currently. Or at least, it should be working for our confirm modals.... if it's not it's a bug.

]);

app.directive('dashboardCloneModal', function (reactDirective) {

This comment has been minimized.

Copy link
@kobelb

kobelb May 16, 2017

Contributor

We should be able to remove this as the showCloneModal is using the React component directly as opposed to via ng-react.

}

componentDidMount() {
this.confirmButton.focus();

This comment has been minimized.

Copy link
@kobelb

kobelb May 16, 2017

Contributor

I haven't used it myself yet, but it looks like there is an autoFocus attribute that we can add to the button itself to make this work.

Tangentially, what made you choose to have the confirm button focused by default. From my naive perspective, we'd want the user to change the name of the Dashboard so there isn't a naming collision, so defaulting the focus to the textbox for them to make the changes would make more sense. This point might no longer be valid now that we're appending "Copy" to the name of the title with the most recent changes.

This comment has been minimized.

Copy link
@stacey-gammon

stacey-gammon May 16, 2017

Author Contributor

I didn't use autoFocus because it's not supported by all browsers here: http://caniuse.com/#feat=autofocus

Though honestly, I thought it wasn't supported on Safari, but a closer look reveals it's "IOS" safari, so the phone browser, and Opera Mini. So maybe it's okay? Since they are touch screen devices, you wouldn't be pressing enter anyway, but touching one of the buttons.

@epixa, do we have a set of browsers we have to support?

This comment has been minimized.

Copy link
@epixa

epixa May 16, 2017

Member

We officially support these browsers: https://www.elastic.co/support/matrix#show_browsers

No need to optimize for mobile browsers, though Kibana should really still function in those browsers, ideally.

This comment has been minimized.

Copy link
@stacey-gammon

stacey-gammon May 16, 2017

Author Contributor

In that case, I'll switch over to using autoFocus, it's a lot simpler of an implementation and Kibana will still function in those mobile browsers.

This was referenced May 16, 2017

@stacey-gammon stacey-gammon force-pushed the stacey-gammon:clone-dashboard-feature branch from 98a4061 to f85070f May 17, 2017

stacey-gammon added some commits Mar 29, 2017

Fix focus issues and tests
Unfortunately can’t run jest tests outside of the ui_framework at this
time.
move the jest tests out of the __tests__ directory
It’ll cause failures for the normal unit test runs

@stacey-gammon stacey-gammon force-pushed the stacey-gammon:clone-dashboard-feature branch from f85070f to 694c5bf May 17, 2017

@stacey-gammon stacey-gammon merged commit 3caf5d7 into elastic:master May 19, 2017

2 checks passed

CLA Commit author has signed the CLA
Details
kibana-ci Build finished.
Details

stacey-gammon added a commit to stacey-gammon/kibana that referenced this pull request May 19, 2017

Introduce Clone feature in view mode (elastic#10925)
* Introduce Clone feature in view mode

* Use a new react modal for cloning dashboards

* Fix focus issues and tests

Unfortunately can’t run jest tests outside of the ui_framework at this
time.

* Add tests for dashboard clone modal

* move the jest tests out of the __tests__ directory

It’ll cause failures for the normal unit test runs

* use react instead of angular for overlay and loading of dom element

* Append 'Copy' to the title in the clone dialog so by default it doesn't clash

* address code comments

stacey-gammon added a commit that referenced this pull request May 19, 2017

Introduce Clone feature in view mode (#10925) (#11923)
* Introduce Clone feature in view mode

* Use a new react modal for cloning dashboards

* Fix focus issues and tests

Unfortunately can’t run jest tests outside of the ui_framework at this
time.

* Add tests for dashboard clone modal

* move the jest tests out of the __tests__ directory

It’ll cause failures for the normal unit test runs

* use react instead of angular for overlay and loading of dom element

* Append 'Copy' to the title in the clone dialog so by default it doesn't clash

* address code comments

@stacey-gammon stacey-gammon deleted the stacey-gammon:clone-dashboard-feature branch May 19, 2017

snide added a commit to snide/kibana that referenced this pull request May 30, 2017

Introduce Clone feature in view mode (elastic#10925)
* Introduce Clone feature in view mode

* Use a new react modal for cloning dashboards

* Fix focus issues and tests

Unfortunately can’t run jest tests outside of the ui_framework at this
time.

* Add tests for dashboard clone modal

* move the jest tests out of the __tests__ directory

It’ll cause failures for the normal unit test runs

* use react instead of angular for overlay and loading of dom element

* Append 'Copy' to the title in the clone dialog so by default it doesn't clash

* address code comments

Dreadnoth added a commit to Dreadnoth/kibana that referenced this pull request Aug 8, 2017

Introduce Clone feature in view mode (elastic#10925) (elastic#11923)
* Introduce Clone feature in view mode

* Use a new react modal for cloning dashboards

* Fix focus issues and tests

Unfortunately can’t run jest tests outside of the ui_framework at this
time.

* Add tests for dashboard clone modal

* move the jest tests out of the __tests__ directory

It’ll cause failures for the normal unit test runs

* use react instead of angular for overlay and loading of dom element

* Append 'Copy' to the title in the clone dialog so by default it doesn't clash

* address code comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.