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

Always collapse an expanded kind without changing selected story #1590

Merged
merged 2 commits into from
Aug 4, 2017

Conversation

Hypnosphi
Copy link
Member

Currently, the stories hierarchy has a confusing behaviour when you click on an expanded kind. If the kind was selected at the moment of clicking, it collapses, but the selected story changes to the first of kind. Otherwise, the first story is selected without collapsing the kind.

I've changed this behaviour to always collapsing without selected story. I preferred though to keep the behaviour when a collapsed kind is expanded along with selecting the first story: this looks quite useful and usually saves a click.

before (note the change in iframe on first click)
after

@Hypnosphi Hypnosphi requested a review from igor-dv August 3, 2017 21:16
@shilman
Copy link
Member

shilman commented Aug 3, 2017

Not sure what the "best" behavior is, but I agree this looks like an improvement. 👍

@trevoreyre
Copy link
Contributor

Definitely an improvement. I still would prefer that any time you click a kind, it doesn't change the active story, whether expanding or collapsing. That feels like more consistent behavior to me. But I don't know, what does everyone else think?

@Hypnosphi
Copy link
Member Author

I think it depends on what do you do more often: explore the tree, or actually navigate through stories

@trevoreyre
Copy link
Contributor

True. I think my reasoning is that it looks almost identical to file trees in applications like VS Code or Atom, so my first intuition is that it will navigate the same way.

Copy link
Member

@igor-dv igor-dv left a comment

Choose a reason for hiding this comment

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

LGTM.

@@ -261,6 +261,31 @@ describe('manager.ui.components.left_panel.stories', () => {
expect(onSelectStory).toHaveBeenCalledWith('a', null);
});

test('should call the onSelectStory prop when an expanded kind is clicked', () => {
Copy link
Member

Choose a reason for hiding this comment

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

should NOT call

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure

@@ -261,6 +261,31 @@ describe('manager.ui.components.left_panel.stories', () => {
expect(onSelectStory).toHaveBeenCalledWith('a', null);
});

test('should call the onSelectStory prop when an expanded kind is clicked', () => {
const data = createHierarchy([
{ kind: 'a', stories: ['a1', 'a2'] },
Copy link
Member

Choose a reason for hiding this comment

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

CodeFactor warns about these lines are duplicated. Maybe change something here..

Copy link
Member Author

Choose a reason for hiding this comment

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

Will try

Copy link
Member Author

@Hypnosphi Hypnosphi Aug 4, 2017

Choose a reason for hiding this comment

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

Ok, so i extracted common vars in the whole test for consistency

@codecov
Copy link

codecov bot commented Aug 4, 2017

Codecov Report

Merging #1590 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1590   +/-   ##
=======================================
  Coverage   20.41%   20.41%           
=======================================
  Files         241      241           
  Lines        5255     5255           
  Branches      645      655   +10     
=======================================
  Hits         1073     1073           
+ Misses       3695     3676   -19     
- Partials      487      506   +19
Impacted Files Coverage Δ
...les/ui/components/left_panel/stories_tree/index.js 100% <100%> (ø) ⬆️
addons/info/src/components/Node.js 38.88% <0%> (ø) ⬆️
...codemod/src/transforms/update-organisation-name.js 40.62% <0%> (ø) ⬆️
lib/ui/src/modules/ui/libs/hierarchy.js 45.45% <0%> (ø) ⬆️
app/react/src/client/preview/element_check.js 41.17% <0%> (ø) ⬆️
addons/storyshots/src/require_context.js 0% <0%> (ø) ⬆️
lib/ui/src/modules/shortcuts/actions/shortcuts.js 6.25% <0%> (ø) ⬆️
lib/ui/src/modules/ui/configs/handle_keyevents.js 33.33% <0%> (ø) ⬆️
lib/ui/src/modules/ui/containers/down_panel.js 23.52% <0%> (ø) ⬆️
addons/notes/src/react.js 0% <0%> (ø) ⬆️
... and 15 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bfc9c85...6c22e3c. Read the comment docs.

@Hypnosphi Hypnosphi merged commit e3ac03c into master Aug 4, 2017
@Hypnosphi Hypnosphi deleted the collapse-kind branch August 4, 2017 12:10
@andrei-ilyukovich
Copy link

I preferred though to keep the behaviour when a collapsed kind is expanded along with selecting the first story: this looks quite useful and usually saves a click.

@Hypnosphi I couldn't find such behavior in the latest releases, was it removed completely or is there any possibility to turn this on? Also I couldn't find option to make tree view be expanded or collapsed by a single click, I think it would be useful for saving clicks as well.

@Hypnosphi
Copy link
Member Author

@igor-dv decided to remove it

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.

5 participants