Add hrefs to left menu links #1523

Hypnosphi · 2017-07-25T21:02:18Z

This improves UX by making it possible to open stories in new tab/window, copy links etc.

shilman · 2017-07-26T00:42:48Z

@igor-dv mind taking a look at this?

igor-dv · 2017-07-26T04:48:20Z

@Hypnosphi , cool thing, but:

There will be incompatible changes in the left panel structure in upcoming 3.2 release , so I think you should branch from the release/3.2

igor-dv

Let's review after the compatibility changes

ndelangen · 2017-07-28T08:00:24Z

@Hypnosphi Awesome!! Do you think you could resolve the merge conflicts?

Hypnosphi · 2017-07-28T10:35:11Z

Those changes may be non-trivial to integrate with react-treebeard, but I'd give it a try

ndelangen · 2017-07-28T10:43:56Z

If you need help, just let me know! If you'd like to join the storybook slack, you can self-invite here: https://now-examples-slackin-nqnzoygycp.now.sh/

igor-dv · 2017-07-28T11:46:04Z

@Hypnosphi , look at HeaderDecorator in ui/components/left_panel/stories_tree/tree_decorators.js . There is a <a> tag that you can replace.

Hypnosphi · 2017-07-28T16:12:30Z

@igor-dv Sure, but I also need to put onclick handler on that link instead of this div: https://github.com/alexcurtis/react-treebeard/blob/master/src/components/decorators.js#L55

In fact, I think the whole Container should be a link (because it's the clickable area)

codecov · 2017-07-29T01:56:02Z

Codecov Report

Merging #1523 into master will increase coverage by 0.17%.
The diff coverage is 53.08%.

@@            Coverage Diff             @@
##           master    #1523      +/-   ##
==========================================
+ Coverage   20.41%   20.59%   +0.17%     
==========================================
  Files         241      244       +3     
  Lines        5255     5308      +53     
  Branches      654      653       -1     
==========================================
+ Hits         1073     1093      +20     
- Misses       3681     3719      +38     
+ Partials      501      496       -5

Impacted Files	Coverage Δ
...i/components/left_panel/stories_tree/tree_style.js	`25% <ø> (ø)`	⬆️
...les/ui/components/left_panel/stories_tree/index.js	`100% <ø> (ø)`	⬆️
lib/ui/src/modules/ui/containers/routed_link.js	`100% <100%> (ø)`
lib/ui/src/modules/ui/components/routed_link.js	`100% <100%> (ø)`
lib/ui/src/modules/ui/components/menu_item.js	`19.14% <33.33%> (ø)`
...ponents/left_panel/stories_tree/tree_decorators.js	`33.33% <46.87%> (+3.75%)`	⬆️
lib/ui/src/modules/ui/configs/handle_routing.js	`28.04% <66.66%> (-0.9%)`	⬇️
addons/knobs/src/components/types/Number.js	`8.06% <0%> (ø)`	⬆️
addons/knobs/src/components/PropForm.js	`8.51% <0%> (ø)`	⬆️
... and 26 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 9b0dabe...c4bfa31. Read the comment docs.

igor-dv · 2017-07-30T10:02:30Z

@Hypnosphi LMK, when it's ready to review.

Hypnosphi · 2017-07-30T12:40:25Z

@igor-dv I think it is

igor-dv

LGTM,

The only thing I am not confident about is the final blue/black UI.

I think it's too much blue...
IMO making the Component name bold together with a story is a bit confusing..

igor-dv · 2017-07-31T09:14:42Z

lib/ui/src/modules/ui/components/routed_link.js

+
+// Cmd/Ctrl/Shift/Alt + Click should trigger default browser behaviour. Same applies to non-left clicks
+function isPlainLeftClick(e) {
+  return e.button === 0 && !e.altKey && !e.ctrlKey && !e.metaKey && !e.shiftKey;


extract 0 to some const for readability

igor-dv · 2017-07-31T09:32:39Z

lib/ui/src/modules/ui/components/routed_link.js

+  }
+
+  render() {
+    const { onClick, href, children, overrideParams, ...restProps } = this.props;


I assume that you destructure onClick and overrideParams here to not pass them to the a.
If so, consider using something like this ?

const newProps = { ...this.props, onClick: this.onClick, overrideParams: null, }; return ( <a {...newProps}></a> );

Null is also a value. Try and pass it to some dom element, react warns you (and from version 16 it will silently append it as an attribute)

Crap... those unused props are annoying me..

plus we need explicit href and children to make eslint-plugin-jsx-a11y happy

igor-dv · 2017-07-31T09:39:18Z

lib/ui/src/modules/ui/components/theme.js

@@ -1,7 +1,7 @@
 export const baseFonts = {
  fontFamily: `
-    -apple-system, ".SFNSText-Regular", "San Francisco", "Roboto",
-    "Segoe UI", "Helvetica Neue", "Lucida Grande", sans-serif
+    -apple-system, "BlinkMacSystemFont", ".SFNSText-Regular", "San Francisco", "Roboto",


I don't know if it's a good time to do this change, since there a lot of other places that use these fonts.. So it makes it a bit inconsistent..

Well we've discussed in slack in the context of menu items jumping when they go from bold to normal. I can extract it to a separate PR if needed

Yeah, I remember.. IMO it's better to change all these fonts together.. even in concerns of a git history..

igor-dv · 2017-07-31T10:00:34Z

lib/ui/src/modules/ui/components/left_panel/stories_tree/tree_decorators.js

+
+  if (node.type === treeNodeTypes.NAMESPACE) {
+    return (
+      <MenuItem style={containerStyle} onClick={onClick} data-name={node.name}>


is data-name only for tests ?

Basically so

Hypnosphi · 2017-07-31T10:34:10Z

Speaking of the "too much blue" part, we need to indicate somehow that a particular menu item is a link. Stories and kinds are, namespaces aren't. We can replace blue with underlines, but I'm not sure we should.

At the same time, when some kind is already open, its link should be somehow marked as "active". If we just remove color, it becomes undistinguishable from namespace again. That's why bold is needed, but I'm open to other solutions.

@shilman, what do you think?

Hypnosphi · 2017-08-03T03:44:03Z

Maybe we should use links only for stories. In that case kinds wouldn't need to visually differ from namespaces

Hypnosphi · 2017-08-03T04:47:19Z

How do you guys feel about adding indentation for stories by the way?

igor-dv · 2017-08-03T05:09:41Z

@Hypnosphi I like the idea to make stories only blue (or links). IMO selected story shouldn't became black.. maybe some shade of blue ? Indentations is too deep I think, but nice

Are there any designers in here 🆘 ?

Hypnosphi · 2017-08-03T05:24:53Z

story shouldn't became black

Why? It's quite a standard way to mark link as 'active'

shilman · 2017-08-03T06:02:30Z

Here are my opinions on this stuff. Sorry for the slow turnaround @Hypnosphi @igor-dv:

It's great to change things to links to make it easy to open up stories in other windows, accessibility, etc. 👍
It's great to make a better visual distinction between "stories" and "kinds". 👍
I don't think there necessarily needs to be a visual distinction between "folders" and "kinds" (they are all just groupings in the end). 👎
I think the original indentation was fine. I think more indentation is not a good use of space, especially if we make a good visual distinction between stories and kinds. 👎
I think the bright blue is pretty overwhelming. I'd like to come up with a better way to distinguish stories and kinds. Even the more subtle blue is pretty ... blue 👎

Hope that helps! 😀

shilman · 2017-08-03T06:20:33Z

Also to help with your blue/black debate, I submit this. Heheheh.

Personal preference is to keep the existing 3.2 look-and-feel and get another option ready to help distinguish stories and kinds, but I'm ok with subtle changes as long as we can revisit it soon.

ndelangen

My thoughts on these discussions:
✔️ better accessibility
✔️ blue
✔️ black=active
✔️ visual difference kind|story
❌ visual difference kind|group
✔️ added indentation

Screenshot is OK with me.

@shilman the dress is blue & gold

Hypnosphi · 2017-08-03T12:28:07Z

I think the bright blue is pretty overwhelming

Well it's link color from our website

Hypnosphi · 2017-08-03T13:02:35Z

@shilman @igor-dv @ndelangen
Can we maybe compromise on some minor indentation?

Hypnosphi · 2017-08-03T13:13:02Z

As an alternative to color I can suggest slight underline:

shilman · 2017-08-03T14:54:04Z

I think the bright blue is pretty overwhelming

Well it's link color from our website

The website and Storybook are entirely different UI's. Just because it looks OK in one doesn't mean it looks OK in the other.

Hypnosphi · 2017-08-03T15:33:01Z

I tried indigo from material palette

shilman

I'm sure this is tiring, but I'd like to propose that we split the PR into two:

functional changes that i'm happy to merge/release today
visual changes that are a lot more contentious

I have stronger thoughts about the visual side after consulting some designers, but i'd like to get the functional aspect of this merged and shipped

Hypnosphi · 2017-08-03T16:16:02Z

So, to be clear, you want to introduce black links without underline (for now)?

Hypnosphi · 2017-08-03T17:57:46Z

Ok, so I've rebased the branch, and left only the functional changes plus some minor style adjustments needed for correct focus outline dispaying

shilman

LGTM! Thanks for separating out the functional pieces. We'll come up with a good visual solution too!

ndelangen · 2017-08-04T23:10:20Z

🥇

shilman requested a review from igor-dv July 26, 2017 00:42

igor-dv requested changes Jul 26, 2017

View reviewed changes

igor-dv added feature request ui labels Jul 26, 2017

ndelangen assigned Hypnosphi Jul 28, 2017

ndelangen assigned igor-dv Jul 28, 2017

igor-dv reviewed Jul 31, 2017

View reviewed changes

igor-dv approved these changes Jul 31, 2017

View reviewed changes

Hypnosphi requested a review from shilman August 1, 2017 17:04

igor-dv mentioned this pull request Aug 2, 2017

UI: update font family #1560

Merged

ndelangen approved these changes Aug 3, 2017

View reviewed changes

shilman requested changes Aug 3, 2017

View reviewed changes

Add link funtionality to story menu items

bd0524c

storybook-safe-bot and others added 2 commits August 4, 2017 15:11

Merge branch 'master' into menu-hrefs

e033e6b

Adjust the merged test

47e54ed

shilman approved these changes Aug 4, 2017

View reviewed changes

Merge branch 'master' into menu-hrefs

c4bfa31

igor-dv merged commit ab45a9b into storybookjs:master Aug 4, 2017

Hypnosphi deleted the menu-hrefs branch August 4, 2017 20:35

This was referenced Aug 5, 2017

Tree view visual adjustments #1599

Merged

Add z-index to shortcuts popup overlay #1617

Merged

Add hrefs to left menu links #1523

Add hrefs to left menu links #1523

Conversation

Hypnosphi commented Jul 25, 2017

shilman commented Jul 26, 2017

igor-dv commented Jul 26, 2017

igor-dv left a comment

Choose a reason for hiding this comment

ndelangen commented Jul 28, 2017

Hypnosphi commented Jul 28, 2017

ndelangen commented Jul 28, 2017

igor-dv commented Jul 28, 2017

Hypnosphi commented Jul 28, 2017 • edited Loading

codecov bot commented Jul 29, 2017 • edited Loading

Codecov Report

igor-dv commented Jul 30, 2017

Hypnosphi commented Jul 30, 2017

igor-dv left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hypnosphi Jul 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hypnosphi commented Jul 31, 2017 • edited Loading

Hypnosphi commented Aug 3, 2017 • edited Loading

Hypnosphi commented Aug 3, 2017

igor-dv commented Aug 3, 2017

Hypnosphi commented Aug 3, 2017

shilman commented Aug 3, 2017 • edited Loading

shilman commented Aug 3, 2017

ndelangen left a comment • edited Loading

Choose a reason for hiding this comment

Hypnosphi commented Aug 3, 2017

Hypnosphi commented Aug 3, 2017

Hypnosphi commented Aug 3, 2017

shilman commented Aug 3, 2017 • edited Loading

Hypnosphi commented Aug 3, 2017

shilman left a comment

Choose a reason for hiding this comment

Hypnosphi commented Aug 3, 2017 • edited Loading

Hypnosphi commented Aug 3, 2017

shilman left a comment

Choose a reason for hiding this comment

ndelangen commented Aug 4, 2017

Hypnosphi commented Jul 28, 2017 •

edited

Loading

codecov bot commented Jul 29, 2017 •

edited

Loading

igor-dv left a comment •

edited

Loading

Hypnosphi Jul 31, 2017 •

edited

Loading

Hypnosphi commented Jul 31, 2017 •

edited

Loading

Hypnosphi commented Aug 3, 2017 •

edited

Loading

shilman commented Aug 3, 2017 •

edited

Loading

ndelangen left a comment •

edited

Loading

shilman commented Aug 3, 2017 •

edited

Loading

Hypnosphi commented Aug 3, 2017 •

edited

Loading