Skip to content

Conversation

@melvin15may
Copy link
Contributor

Fixes issue #262
Added save button with its own dedicated modal.

Screenshot of layout editor before this PR
BEFORE

Screenshot of layout editor after this PR
AFTER

Screenshot of Save dialog
Save dialog modal


},
saveLayout: function (layoutName) {
saveLayout: function (layoutName, modalNameId) {
Copy link
Collaborator

@adbharadwaj adbharadwaj May 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont like the change to saveLayout method. It doesn't make sense in terms of the method signature. Here are few questions for you -

  • Can we rename modalNameId to a more intuitive name?
  • Can we move modalNameID out of saveLayout method? It seems that we need it only for closing the modal.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can rename modalNameId to something more intuitive. Any suggestions?

I need the ID of the modal element to toggle between the element's state only on successful callback. If I take it out will have to implement a callback (callback from a callback situation). I thought of making it modular, in case we use this function for other DOM elements.

@jlaw9
Copy link
Collaborator

jlaw9 commented Aug 22, 2017

The current title of "Save Layout" leads users to believe they are saving the layout of the nodes (x and y positions) rather than the x and y positions and the style of every node which leads to issues like #310. I suggest we change the save popup title to "Save Layout and Style" or add a description in the popup saying something like "Current x and y positions of nodes as well as style attributes of every node and edge will be saved"

@tmmurali
Copy link
Member

tmmurali commented Aug 22, 2017 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants