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

COMPASS-419: Expand all/collapse all in document #663

Merged
merged 4 commits into from Dec 11, 2016
Merged

Conversation

durran
Copy link
Member

@durran durran commented Dec 4, 2016

Didn't know exactly how to handle this from a design perspective, so I added 2 buttons, an "expand all" button and "collapse all" button next to the other document actions that appear on hover of the document. I used font awesome's Double Angle Down and Double Angle Up icons.

cc/ @fredtruman @Sean-Oh

Here's the behaviour:
dec-04-2016 17-48-39

@Sean-Oh
Copy link
Contributor

Sean-Oh commented Dec 5, 2016

This mostly LGTM.
I changed the caret icons to the following:
screen shot 2016-12-05 at 1 22 13 pm
One recommendation: Is it possible to have only one button at a time that toggles between collapse and expand functions? Maybe we can switch the IconButtons depending on the state of expandAll.

@rueckstiess
Copy link
Member

Agree with Sean one button that changes its icon from down-arrow (expanded) to side-arrow (collapsed, note: not up-arrow!) would probably suffice. If we add multiple buttons to the toolbar for such small features we will quickly look like Microsoft Word :-)

@Sean-Oh
Copy link
Contributor

Sean-Oh commented Dec 6, 2016

Good catch! Definitely should be a side-arrow to collapse and a down-arrow to expand.

@fredtruman
Copy link
Contributor

Maybe since Durran is out, do you guys think we can merge as is and address with separate tickets for style pass?

@samweaver-zz
Copy link

samweaver-zz commented Dec 6, 2016 via email

@Sean-Oh
Copy link
Contributor

Sean-Oh commented Dec 6, 2016

I took a stab at it last night, but couldn't figure out how to get the expandAll state values to pass down properly. I'm not very good with react haha. An engineer could probably do this very quickly though.

@samweaver-zz
Copy link

samweaver-zz commented Dec 6, 2016 via email

@fredtruman
Copy link
Contributor

Sounds good

@durran
Copy link
Member Author

durran commented Dec 9, 2016

Using 1 button increases the scope of this feature tremendously and also would come with potential performance implications on deeply nested documents. Consider the situation:

User clicks the expand all button. All the elements in the document are expanded. The icon switches to collapse all. The user manually collapses each nested field one by one. Now they want to expand them all again.

If we want the button to be intuitive, then which icon it displays depends on the state of all expandable elements all the way down the tree of the document. It would need to track this state and adds a level of complexity here that I think is unnecessary.

If we don't care, then it can be simple and simply toggle back and forth between the two. The case above would be that the user would have to click the button twice (once to collapse all, even though they have already collapsed them all) and then once again to expand all.

@rueckstiess
Copy link
Member

If we don't care, then it can be simple and simply toggle back and forth between the two. The case above would be that the user would have to click the button twice (once to collapse all, even though they have already collapsed them all) and then once again to expand all.

I think that's ok. This is a common pattern. The button just has two states and toggles between them. Even if the user manually starts collapsing fields we don't track or measure that. The button just switches between expandAll and collapseAll. You can always click twice to get to the desired state.

@durran
Copy link
Member Author

durran commented Dec 11, 2016

@fredtruman @Sean-Oh @rueckstiess Updated to use just one button:

dec-11-2016 15-18-25

@durran durran merged commit c572191 into master Dec 11, 2016
@durran durran deleted the COMPASS-419 branch December 11, 2016 20:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants