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

Tree view visual adjustments #1599

Merged
merged 13 commits into from
Aug 10, 2017
Merged

Tree view visual adjustments #1599

merged 13 commits into from
Aug 10, 2017

Conversation

Hypnosphi
Copy link
Member

@Hypnosphi Hypnosphi commented Aug 5, 2017

See discussion in #1523

This PR consists of several changes, which can be applied quite independently. I'll describe each of them in a separate comment, so that you can vote with emoji reactions.

@Hypnosphi
Copy link
Member Author

Some minor indentation was added to stories to highlight the hierarchy

Before
screen shot 2017-08-05 at 03 51 02

After
screen shot 2017-08-05 at 03 37 34

@Hypnosphi
Copy link
Member Author

Link color from Welcome story was used to indicate that story menu items are proper links

Before
screen shot 2017-08-05 at 03 43 49

After
screen shot 2017-08-05 at 03 20 01

@Hypnosphi
Copy link
Member Author

As an alternative to the previous one, I can suggest slight underlines
screen shot 2017-08-05 at 03 29 12

@Hypnosphi
Copy link
Member Author

Line heights for items were made lower than the items mininal height, so that it's easy to distinguish an item with wrapped lines from two consecutive items. Story's minimal height was lowered proportionally to the font size.

Before
screen shot 2017-08-05 at 03 45 50

After
screen shot 2017-08-05 at 03 34 45

@Hypnosphi
Copy link
Member Author

Hypnosphi commented Aug 5, 2017

The items' text is vertically aligned by lowercase letters, so that ascenders and descenders have both enough space

Before
screen shot 2017-08-05 at 04 27 41
screen shot 2017-08-05 at 04 27 29

After
screen shot 2017-08-05 at 04 27 04
screen shot 2017-08-05 at 04 26 44

@Hypnosphi
Copy link
Member Author

Finally, the chevron's alignment and rotation centre were adjusted

Before
After

@trevoreyre
Copy link
Contributor

The blue/underlined links both seem a little too intense to me. I think sidebar navigation is a common enough UI pattern that making the links stand out that much is unnecessary. If anything, maybe a slightly lighter font color for story links? Or perhaps kinds could be bold, and story links normal weight?

Something more along the lines of this, perhaps?

sidebar nav

@Hypnosphi
Copy link
Member Author

screen shot 2017-08-05 at 06 51 19

@igor-dv igor-dv closed this Aug 5, 2017
@shilman
Copy link
Member

shilman commented Aug 5, 2017

@igor-dv did you mean to close this?

@igor-dv igor-dv reopened this Aug 5, 2017
@codecov
Copy link

codecov bot commented Aug 5, 2017

Codecov Report

Merging #1599 into master will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1599      +/-   ##
==========================================
+ Coverage   21.32%   21.34%   +0.01%     
==========================================
  Files         244      244              
  Lines        5378     5378              
  Branches      662      652      -10     
==========================================
+ Hits         1147     1148       +1     
- Misses       3725     3745      +20     
+ Partials      506      485      -21
Impacted Files Coverage Δ
...i/components/left_panel/stories_tree/tree_style.js 25% <ø> (ø) ⬆️
app/react-native/src/bin/storybook-start.js 0% <0%> (ø) ⬆️
...react-native/src/manager/components/PreviewHelp.js 0% <0%> (ø) ⬆️
app/react/src/client/preview/element_check.js 41.17% <0%> (ø) ⬆️
addons/knobs/src/components/types/Object.js 5.81% <0%> (ø) ⬆️
lib/ui/src/modules/shortcuts/actions/shortcuts.js 6.25% <0%> (ø) ⬆️
.../ui/src/modules/ui/components/left_panel/header.js 28.57% <0%> (ø) ⬆️
...b/ui/src/modules/ui/components/left_panel/index.js 100% <0%> (ø) ⬆️
addons/knobs/src/react/WrapStory.js 12% <0%> (ø) ⬆️
lib/ui/src/modules/ui/components/layout/usplit.js 38.7% <0%> (ø) ⬆️
... and 19 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 f21210a...c859985. Read the comment docs.

@trevoreyre
Copy link
Contributor

Yeah, the dark grey story links looks much better IMO.

@Hypnosphi
Copy link
Member Author

Just for the record, the current branch state contains all the upvoted changes, without the downvoted ones =)

Copy link
Member

@danielduan danielduan left a comment

Choose a reason for hiding this comment

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

Changing the look and feel of something existing is always gonna be controversial. I'm pretty comfortable with the changes here, think this would add clarity.

@danielduan danielduan changed the title Tree view visual adjusments Tree view visual adjustments Aug 9, 2017
Copy link
Member

@ndelangen ndelangen left a comment

Choose a reason for hiding this comment

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

Looks good: ✅
Locally tested: ❌

@ndelangen
Copy link
Member

Anyone opposed to merging this? ( @shilman )

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

@ndelangen ndelangen added the maintenance User-facing maintenance tasks label Aug 10, 2017
Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

🎉

@ndelangen ndelangen merged commit 1e2855c into master Aug 10, 2017
@ndelangen ndelangen deleted the tree-visual-adjusments branch August 10, 2017 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance User-facing maintenance tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants