feat(Sidebar): add component #905

fracmak · 2016-11-20T05:51:01Z

Fixes #197, Replaces PR #774.

Adds component with examples

codecov-io · 2016-11-20T05:56:00Z

Current coverage is 99.74% (diff: 78.57%)

Merging #905 into master will decrease coverage by 0.25%

@@           master       #905   diff @@
========================================
  Files         137        140     +3   
  Lines        2286       2314    +28   
  Methods         0          0          
  Messages        0          0          
  Branches        0          0          
========================================
+ Hits         2286       2308    +22   
- Misses          0          6     +6   
  Partials        0          0

Powered by Codecov. Last update 17fbdf7...a403966

levithomason · 2016-11-20T23:52:32Z

Wow, thanks! I'll get a review on this as soon as I can 😸

kevflynn · 2016-11-21T13:03:42Z

So excited for this one 🐶

kevflynn · 2016-11-22T13:31:32Z

@levithomason - I know you're super-busy - any chance you can review this week? 😁

levithomason

We need to figure out an API for the pushable/pusher. Given that these can appear anywhere in the tree, my first thought is a component/subcomponent. I believe it is only applicable to the Sidebar, so perhaps Sidebar subcomponents would make sense?

<Sidebar.Pushable>
  ...
  <Sidebar />
  <Sidebar.Pusher />
<Sidebar.Pushable />

This also follows our <Dimmer.Dimmable /> pattern and allows us to add props and features to each individually.

Pushable `body`

There is also the need to make the body element pushable for full page Sidebars. We could again borrow from the Dimmer.Dimmable component and provide a page prop which, like a page Dimmer affects the body, it would make the entire page pushable:

<Sidebar.Pushable page />

This might even make more sense as a Sidebar prop, since there is no need to place the "pushable" element in the tree and no need to support "pushable" children since it is only adding/removing the className to the body:

<Sidebar page />

This would also need to render the Sidebar to the body using the Portal. It also would need to render a pusher to the body.

Inspecting SUI core docs for full page Sidebars, this might be problematic. The pusher needs to wrap the React app mount node in order to work. I'm not sure on how to go about this since modifying React's render tree will break it. I'll leave it at this for now. Here is the markup required by SUI, per the core docs:

<body class="pushable">
  <div class="sidebar"></div>
  <div class="pusher">
      <div class="root">my react app</div>
  </div>
</body>

Given the tricky issues here, I'm OK adding page level pushers on a separate PR as I suspect it is going to be fairly involved.

Pusher

I think the <Sidebar.Pusher /> API makes sense in and applies to all use cases here. It seems it is always required. LMK if you disagree or see a better API.

Dimmed Pusher

The pusher can also be dimmed, http://semantic-ui.com/modules/sidebar#dimmed. My first thought on this is <Sidebar.Pusher dimmed />. Let's add this for now.

I think in an ideal workflow something like a <Pusher dimmable /> would dim itself when the Sidebar was active. I have a couple ideas for this, but again, let's consider auto dimming on a separate PR and this one will just add the simple prop dimmed.

levithomason · 2016-11-22T16:57:42Z

I've left an initial review, this looks really great so far, thanks!

fracmak · 2016-11-23T07:32:36Z

Ah, I like the suggestions, I'll get on making them. Though with the holidays it might have to wait until next week. Hopefully I'll get to it sooner. I'll comment when it's ready for another review

fracmak · 2016-11-28T03:28:14Z

Added Sidebar.Pushable and Sidebar.Pusher with docs.

levithomason · 2016-12-03T02:06:14Z

Dynamic `animating` className

I've updated the Sidebar to use a dynamic animating className. This was the reason all the inline overflow styles were required. I didn't want users thinking that they had to add inline styles to get it to work, so I removed them all.

Using Segments

I also updated Pushables in all examples to augment the Segment and added a basic Segment child to the Pusher. This was the reason all the inline background styles were required. I remove those inline styles as well. This will be handled automatically once #813 is merged.

I've based this markup here off of the examples here: http://semantic-ui.com/modules/sidebar.html#/examples. The Pushable is a Segment while the Pusher first child is a basic Segment. This gives the proper styling and alignment for non page level Sidebars.

levithomason · 2016-12-03T02:09:00Z

Releasing this now, massive thanks @UnbrandedTech!

UnbrandedTech · 2016-12-03T02:17:01Z

Don't thank me, thank @fracmak. He Literally did all the work!

levithomason · 2016-12-03T02:40:13Z

Oh snap! Getting my friends confused there :D Thanks @fracmak!

fracmak · 2016-12-03T03:05:40Z

woo! excited to see this land!

vmptk · 2016-12-05T14:31:23Z

Any good practice how to hide the sidebar when click on any place outside its area and on a menu item?

fracmak · 2016-12-05T14:55:53Z

I would say using a click handler on the outer element that's controlling the visibility of the sidebar would be enough since click events bubble. You can then add a click handler to the sidebar itself to preventDefault() if the click wasn't on a link.

fracmak · 2016-12-05T14:58:59Z

src/collections/Sidebar/Sidebar.js

+
+    this.setState({ animating: true })
+
+    this.stopAnimatingTimer = setTimeout(() => this.setState({ animating: false }), duration)


I was thinking about it, any reason we aren't using the transitionend event to trap the animation finishing instead of hard coding 500ms?

I actually did not know about this event, thanks for pointing it out. The primary reason I added this hack was because we have a Transition component that will soon be merged and take over this task, #813.

Support for this event seems solid, I'd merge a PR updating this bit of code if you want to open one.

I'll work on this later this week, there's one gotcha with the event, if the tab/browser isn't active, those events never fire, so you have to be careful with handling that edgecase

$fracmak$

$@fracmak$ fracmak force-pushed the sidebar branch 2 times, most recently from c488d15 to ecabd5f Compare November 20, 2016 13:55

levithomason added the ready for review label Nov 20, 2016

levithomason requested changes Nov 22, 2016

View reviewed changes

levithomason added needs author feedback and removed ready for review labels Nov 22, 2016

This was referenced Nov 22, 2016

feat(Sidebar): add component #774

Closed

<Sidebar /> Component #197

Closed

levithomason added ready for review and removed needs author feedback labels Nov 29, 2016

fracmak added 3 commits December 1, 2016 12:14

$@fracmak$

feat(Sidebar): add component

dd4c9ed

$@fracmak$

feat(Sidebar): add component examples

d286850

$@fracmak$

feat(Sidebar): added Sidebar.Pushable and Sidebar.Pusher with docs

367d7d6

$@fracmak$ fracmak force-pushed the sidebar branch from 171dad4 to 367d7d6 Compare December 1, 2016 17:14

levithomason added 2 commits December 2, 2016 18:00

fix(Sidebar): use dynamic animating className

35b9429

docs(Sidebar): use augmentation and remove inline styles

a403966

levithomason merged commit d935f9b into Semantic-Org:master Dec 3, 2016

$fracmak$

fracmak commented Dec 5, 2016

View reviewed changes

UnbrandedTech mentioned this pull request Mar 1, 2017

feat(Sidebar): Full page sidebar #1408

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(Sidebar): add component #905

feat(Sidebar): add component #905

$@fracmak$ fracmak commented Nov 20, 2016 •

edited by levithomason

Loading

codecov-io commented Nov 20, 2016 •

edited

Loading

levithomason commented Nov 20, 2016

kevflynn commented Nov 21, 2016

kevflynn commented Nov 22, 2016

levithomason left a comment

levithomason commented Nov 22, 2016

fracmak commented Nov 23, 2016

fracmak commented Nov 28, 2016

levithomason commented Dec 3, 2016

levithomason commented Dec 3, 2016

UnbrandedTech commented Dec 3, 2016

levithomason commented Dec 3, 2016

fracmak commented Dec 3, 2016

vmptk commented Dec 5, 2016

fracmak commented Dec 5, 2016

$@fracmak$ fracmak Dec 5, 2016

levithomason Dec 5, 2016

$@fracmak$ fracmak Dec 6, 2016


		this.setState({ animating: true })

		this.stopAnimatingTimer = setTimeout(() => this.setState({ animating: false }), duration)

feat(Sidebar): add component #905

feat(Sidebar): add component #905

Conversation

fracmak commented Nov 20, 2016 • edited by levithomason Loading

codecov-io commented Nov 20, 2016 • edited Loading

Current coverage is 99.74% (diff: 78.57%)

levithomason commented Nov 20, 2016

kevflynn commented Nov 21, 2016

kevflynn commented Nov 22, 2016

levithomason left a comment

Choose a reason for hiding this comment

Pushable body

Pusher

Dimmed Pusher

levithomason commented Nov 22, 2016

fracmak commented Nov 23, 2016

fracmak commented Nov 28, 2016

levithomason commented Dec 3, 2016

Dynamic animating className

Using Segments

levithomason commented Dec 3, 2016

UnbrandedTech commented Dec 3, 2016

levithomason commented Dec 3, 2016

fracmak commented Dec 3, 2016

vmptk commented Dec 5, 2016

fracmak commented Dec 5, 2016

fracmak Dec 5, 2016

Choose a reason for hiding this comment

levithomason Dec 5, 2016

Choose a reason for hiding this comment

fracmak Dec 6, 2016

Choose a reason for hiding this comment

$@fracmak$ fracmak commented Nov 20, 2016 •

edited by levithomason

Loading

codecov-io commented Nov 20, 2016 •

edited

Loading

Pushable `body`

Dynamic `animating` className

$@fracmak$ fracmak Dec 5, 2016

$@fracmak$ fracmak Dec 6, 2016