feat(Dimmer): Add component #447

layershifter · 2016-08-28T15:09:54Z

Fixes #190

layershifter · 2016-08-28T15:20:41Z

Component

It seems that Dimmer component might be AutocontrolleComponent, if we want add events from settings.

Simple dimmer

SUI offers Simple dimmer in native CSS, but seems it's not part of Dimmer component.

Dimmer events

Any ideas how we can handle events in automatic mode like there?

Usage

I think that in this part there is nothing that should be in the component.

Settings

I think we need take following from settings part:

closable prop, looks usable with page dimmer;
events onShow, onHide, onChange - very debatable idea, if we skip these, we can leave component stateless.

@levithomason Other ideas?

layershifter · 2016-09-10T05:42:22Z

ping @levithomason

levithomason · 2016-09-12T16:37:09Z

Responses

Component

I think we first go with a stateless component here. The active prop will be controlled from the parent component. Once we get that, then we can consider if anything in the Dimmer warrants auto controlling its own state, but I don't think it will.

Simple dimmer

There is actually no difference in markup between the "simple css dimmer" and a regular dimmer. The only difference is in how you use it. The jQuery dimmer simply adds the dimmable dimmed classes to the parent of the dimmer and the active classes to the dimmer itself. A simple css dimmer just means the user added the classes to the parent and dimmer child manually.

Dimmer events

Since the component will be controlled by parent state, I don't think we need events. Let's leave those for future feature requests, if needed.

Usage

Agreed, let's leave these out.

Settings

closable - How about closeOnClick? This way we can add other close settings like closeOnEscape if we want.

events - yes, let's skip and go stateless

API Ideas

The parent component can be any component and also needs dimmable dimmed classes. It must have a <div class="ui dimmer" /> as an immediate child for the CSS to work.

The dimmer itself can also have child content, which means we'll need an API for specifying the dimmed component's children and the dimmer's children.

The dimmer centers content horizontally and vertically. Vertical centering requires two child divs. I think we include these two divs in all dimmer content.

Content must be included inside .content .center to display centered correctly in the dimmer.

I'm proposing that the Dimmer have an API to control both the dimmed parent component as well as the dimmer child.

Content Dimmer

Proposing the dimmable/dimmed props be used strictly for the parent component.

<Dimmer dimmable={Segment} dimmed>
  <Header>Overlayable Section</Header>
  <Dimmer active>
    <Header inverted icon='heart' content='Dimmed Message!' />
  </Dimmer>
</Dimmer>

<div class="ui segment dimmable dimmed">
  <h3 class="ui header">
    Overlayable Section
  </h3>
  <div class="ui dimmer transition visible active">
    <div class="content">
      <div class="center">
        <h2 class="ui inverted icon header">
          <i class="heart icon"></i>
          Dimmed Message!
        </h2>
      </div>
    </div>
  </div>
</div>

Page Dimmer

In this case the body must receive the dimmable dimmed classes and the ui dimmer must an immediate child to the body. This would use a Portal as was done in the Modal dimmer. The Portal provides a div as an immediate child to the body.

<Dimmer page active>
  <Header inverted icon='mail' content='Dimmer Message' subheader='Dimmer sub-header' />
</Dimmer>

<body>
  <div class="ui page dimmer transition visible active">
    <div class="content">
      <div class="center">
        <h2 class="ui inverted icon header">
          <i class="mail icon"></i>
          Dimmer Message
          <div class="sub header">Dimmer sub-header</div>
        </h2>
      </div>
    </div>
  </div>
</body>

layershifter · 2016-10-01T10:49:37Z

I will start on it next week 🍰

layershifter · 2016-10-06T15:28:40Z

Component

After use in production, I can agree about stateless Dimmer 👍

I think that it will be class (will have handleClick, handleEscape), but will not have real state:

export default class Dimmer {
  handleClick = () => {}
  handleEscape = () => {}
  render () {}
}

Dimmable

In this case the body must receive the dimmable dimmed classes

It's real pain there, any ideas about body? I want following syntax, but it looks like impossible mission 😮

<Segment>
  <Dimmer active={false}>...</Dimmer>
  ...
</Segment>

There is simple solution, but it's pretty shitty:

const active = true;

<Segment className={Dimmer.dimmable(active)}>
  <Dimmer active={active}>...</Dimmer>
  ...
</Segment>

@levithomason @jcarbo Please advise me something better 🤐

levithomason · 2016-10-06T19:39:13Z

It's real pain there, any ideas about body? I want following syntax, but it looks like impossible mission 😮

Note, there are 2 types of Dimmers. A "Content Dimmer" and a "Page Dimmer".

The "Page Dimmer" has the body as it's parent and needs to add classes to the body. However, the "Content Dimmer" has any component as it's parent and adds classes to it's parent (your use case above).

I've proposed an API for each use case in #447 (comment) above. See above for more, specifics are here:

Content Dimmer

Renders the dimmable component in place, much like a trigger for a Modal. You can then define a Dimmer as a child, which is shown when the dimmed prop is true.

<Dimmer dimmable={Segment} dimmed />

Page Dimmer

Renders to body.

<Dimmer page active />

Conclusion

I think we'll need to render with these two different approaches based on props:

page - renders to body as a page level dimmer
dimmable - renders the dimmable component in place, passing extra props to it
otherwise - renders the Dimmer markup

codecov-io · 2016-10-10T18:16:36Z

Current coverage is 99.48% (diff: 100%)

No coverage report found for master at 99f2bbf.

Powered by Codecov. Last update 99f2bbf...052aba9

…React into feat/dimmer

layershifter · 2016-10-14T10:47:56Z

Page

Dimmable/Dimmed

SUI adds dimmable dimmed classes to body, it makes window non-scrollable.

I've done this, any better solution there?

handlePageMount = () => {
    debug('handleMount()')
    document.body.classList.add('dimmed', 'dimmable')
  }

  handlePageUnmount = () => {
    debug('handleUnmount()')
    document.body.classList.remove('dimmed', 'dimmable')
  }
}

<Portal onMount={this.handlePageMount} onUnmount={this.handlePageUnmount} />

Handle clickOutside

There is problem with handling outside click. I can't use there Portal's closeOnDocumentClick because Dimmer has 100% width and 100% height.

So, I need handle it inside component, but I can't simply write Dimmer onClick={()} because this handler must omit Dimmer's children (according to SUI examples). Any ideas there?

jeffcarbs · 2016-10-14T13:13:01Z

@layershifter - closeOnDocumentClick actually includes the dimmer. It basically means "any click not within the portal content".

Also once #666 is merged you will have a little more control in that regard. You can either specify closeOnDocumentClick which is the default, or closeOnRootNodeClick which basically means "click on the portal wrapper/root node".

levithomason · 2016-10-14T22:25:47Z

closeOnDocumentClick actually includes the dimmer.

@jcarbo is this due to event bubbling? If so, that makes sense because the Dimmer click bubbles to the document where the event handler is actually attached. If however the portal node (div appended to the body) is receiving the click, I think we should go with something like closeOnClickOutside, if the actual document object is not where the event listener is attached.

Could you clarify?

P.S. I also left a comment on #666 regarding closing multiple Modals, LMK.

layershifter · 2016-10-19T14:46:58Z

@levithomason I'll added dimmable, I've splitted it into separate component. Can you take a look there (DimmerDimmable and blurring example)?

I don't like that we need handle also dimmed prop, any ideas there?

levithomason · 2016-10-19T20:32:00Z

docs/app/Examples/modules/Dimmer/States/DimmerExampleDisabled.js

+
+const DimmerExampleDisabled = () => (
+  <Segment>
+    <Dimmer disabled />


Hm, do you know what this is supposed to do? The dimmer still is made active when I add:

<Dimmer active disabled />

Though, the description says:

A disabled dimmer cannot be activated

Perhaps this is only applicable to the jQuery plugin in SUI core? Thoughts?

I think we can remove disabled prop, it's quite strange to use disabled when you have control of active.

Agreed, especially since it does not override the active prop. Let's remove it.

levithomason · 2016-10-19T20:42:13Z

docs/app/Examples/modules/Dimmer/Variations/DimmerExampleBlurring.js

+
+    return (
+      <div>
+        <Dimmer blurring dimmable={Segment} dimmed={active}>


This currently does not pass extra props to the Segment. Example, adding color='red' should pass color to the Segment. What if we went with this API, it does pass extra props:

<Dimmer.Dimmable as={Segment} blurring dimmed={active} color='red'> <Dimmer active={active} /> <p> <img src='http://semantic-ui.com/images/wireframe/short-paragraph.png' /> </p> <p> <img src='http://semantic-ui.com/images/wireframe/short-paragraph.png' /> </p> </Dimmer.Dimmable>

I tested the above with only these changes required:

Expose Dimmer.Dimmable = DimmerDimmable

Rename DimmmerDimmable prop Component to as and use our standard getElementType() util and render <ElementType /> instead.

I did not do this, but we should then remove all dimmable prop reference and logic from the main Dimmer.

This might be more in line with our existing patterns.

Yep, you said that I thinked 👍

levithomason · 2016-10-19T20:44:29Z

src/modules/Dimmer/Dimmer.js

+  handlePageMount = () => {
+    debug('handleMount()')
+
+    document.body.classList.add('dimmed', 'dimmable')


I believe we only want to add dimmed if active is true. Is that correct?

Not exactly, it added on Portals mount, while Portal will mount on active. This was plan.

levithomason · 2016-10-19T20:45:55Z

Left some comments. I think we must handle dimmed since it is on a child component that can appear anywhere in the child tree.

layershifter · 2016-10-21T13:05:00Z

I've made cleanup there, all examples where added, except example with Image. I was right when I thought that we will need children on Image.

Seems, we need initial review there, I'll add tests after it.

…React into feat/dimmer

layershifter · 2016-10-23T11:10:50Z

Updated to latest master, completed TODOs and added tests 😸

@levithomason waiting for review 😄 All is ready except, common.rendersChildren(), can yoo take a look there?

layershifter · 2016-10-25T13:20:09Z

Added new example with Image and shorthands for it (some style refactor on Image, too).

levithomason · 2016-10-26T04:29:51Z

src/elements/Image/Image.js

 import ImageGroup from './ImageGroup'
+import Dimmer from '../../modules/Dimmer/Dimmer'
+import Label from '../Label/Label'


Just an FYI. There should be index files for all components so we can import ../Label instead of ../Label/Label. We can merge this as is still.

levithomason

Looks good, left one question regarding stopPropagation. I'm also not sure what you'd like to review on rendersChildren(), please expound.

levithomason · 2016-10-26T04:34:13Z

src/modules/Dimmer/Dimmer.js

-      </DimmerDimmable>
-    )
+    _.invoke(children, 'props.onClick', e)
+    e.stopPropagation()


Do we need to stop propagation here? Would prefer we not prevent handlers above this component from firing.

If we're trying to prevent our own handlers from firing, perhaps we should instead detect if the click was inside rather than stop the event.

levithomason · 2016-10-26T04:38:14Z

test/specs/modules/Dimmer/Dimmer-test.js

+  common.hasUIClassName(Dimmer)
+
+  // TODO: Renders children / content
+  // common.rendersChildren()


I saw your request to take a look here, but I'm not sure what the issue is. Did this test not work for this component?

Yes, this test is broken. I think problem in cloneElement that I use for handling click. I'll try rework click handling without stopPropagation.

layershifter · 2016-10-26T18:41:36Z

stopPropagation replaced with ref, fixed example and corrected tests. Ready to go after review 😄

layershifter · 2016-10-26T18:45:23Z

src/modules/Dimmer/Dimmer.js

+
+    if (onClick) onClick(e, this.props)
+    if (this.center && (this.center !== e.target && this.center.contains(e.target))) return
+    if (onClickOutside) onClickOutside(e, this.props)


onClickOutside must be called outside children like in SUI.

layershifter · 2016-10-31T10:36:13Z

@levithomason Any chance for review? 😺

levithomason · 2016-10-31T18:30:28Z

Thanks much!

levithomason · 2016-10-31T19:22:33Z

Released in [email protected]

layershifter added the enhancement label Aug 29, 2016

levithomason mentioned this pull request Sep 27, 2016

RFC: Could/should the modal rely more heavily on React.Portal? #553

Closed

levithomason added the needs author feedback label Oct 9, 2016

Alexander Fedyashov added 2 commits October 10, 2016 21:08

wip

096db63

wip

df1c96a

layershifter force-pushed the feat/dimmer branch from 99b85ab to df1c96a Compare October 10, 2016 18:16

layershifter added 2 commits October 10, 2016 21:25

merge

0b56bc5

fix(Dimmer): update to latest patterns

fb78fde

layershifter mentioned this pull request Oct 11, 2016

Release: Roadmap #657

Closed

15 tasks

Alexander Fedyashov added 4 commits October 14, 2016 10:43

Merge branch 'master' of https://github.com/Semantic-Org/Semantic-UI-…

a17839e

…React into feat/dimmer

feat(Dimmer): create component

e7d8372

feat(Dimmer): create component

bcf514e

feat(Dimmer): create component

332aed5

levithomason removed the needs author feedback label Oct 14, 2016

feat(Dimmer): dimmable

a96322d

levithomason requested changes Oct 19, 2016

View reviewed changes

feat(Dimmer): dimmable

b35317d

layershifter changed the title ~~feat (Dimmer): Add component~~ feat(Dimmer): Add component Oct 23, 2016

layershifter added 2 commits October 23, 2016 14:04

Merge branch 'master' of https://github.com/Semantic-Org/Semantic-UI-…

767cd04

…React into feat/dimmer

feat(Dimmer): add tests, complete TODOs

efb9604

layershifter added the ready for review label Oct 23, 2016

feat(Dimmer): Add shorthands, add example

c65032e

levithomason reviewed Oct 26, 2016

View reviewed changes

fix(Dimmer): fix review comments

052aba9

layershifter commented Oct 26, 2016

View reviewed changes

levithomason approved these changes Oct 31, 2016

View reviewed changes

levithomason merged commit 5bb04d2 into master Oct 31, 2016

levithomason deleted the feat/dimmer branch October 31, 2016 18:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(Dimmer): Add component #447

feat(Dimmer): Add component #447

layershifter commented Aug 28, 2016 •

edited

Loading

layershifter commented Aug 28, 2016 •

edited

Loading

layershifter commented Sep 10, 2016

levithomason commented Sep 12, 2016

layershifter commented Oct 1, 2016

layershifter commented Oct 6, 2016 •

edited

Loading

levithomason commented Oct 6, 2016 •

edited

Loading

codecov-io commented Oct 10, 2016 •

edited

Loading

layershifter commented Oct 14, 2016

jeffcarbs commented Oct 14, 2016

levithomason commented Oct 14, 2016 •

edited

Loading

layershifter commented Oct 19, 2016

levithomason Oct 19, 2016

layershifter Oct 20, 2016

levithomason Oct 20, 2016

levithomason Oct 19, 2016

layershifter Oct 20, 2016

levithomason Oct 19, 2016

layershifter Oct 21, 2016

levithomason commented Oct 19, 2016

layershifter commented Oct 21, 2016

layershifter commented Oct 23, 2016

layershifter commented Oct 25, 2016 •

edited

Loading

levithomason Oct 26, 2016 •

edited

Loading

levithomason left a comment

levithomason Oct 26, 2016

levithomason Oct 26, 2016

layershifter Oct 26, 2016

layershifter commented Oct 26, 2016 •

edited

Loading

layershifter Oct 26, 2016

layershifter commented Oct 31, 2016

levithomason commented Oct 31, 2016

levithomason commented Oct 31, 2016

feat(Dimmer): Add component #447

feat(Dimmer): Add component #447

Conversation

layershifter commented Aug 28, 2016 • edited Loading

layershifter commented Aug 28, 2016 • edited Loading

Component

Simple dimmer

Dimmer events

Usage

Settings

layershifter commented Sep 10, 2016

levithomason commented Sep 12, 2016

Responses

Component

Simple dimmer

Dimmer events

Usage

Settings

API Ideas

Content Dimmer

Page Dimmer

layershifter commented Oct 1, 2016

layershifter commented Oct 6, 2016 • edited Loading

Component

Dimmable

levithomason commented Oct 6, 2016 • edited Loading

Content Dimmer

Page Dimmer

Conclusion

codecov-io commented Oct 10, 2016 • edited Loading

Current coverage is 99.48% (diff: 100%)

layershifter commented Oct 14, 2016

Page

Dimmable/Dimmed

Handle clickOutside

jeffcarbs commented Oct 14, 2016

levithomason commented Oct 14, 2016 • edited Loading

layershifter commented Oct 19, 2016

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

levithomason commented Oct 19, 2016

layershifter commented Oct 21, 2016

layershifter commented Oct 23, 2016

layershifter commented Oct 25, 2016 • edited Loading

levithomason Oct 26, 2016 • edited Loading

Choose a reason for hiding this comment

levithomason left a 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

layershifter commented Oct 26, 2016 • edited Loading

Choose a reason for hiding this comment

layershifter commented Oct 31, 2016

levithomason commented Oct 31, 2016

levithomason commented Oct 31, 2016

layershifter commented Aug 28, 2016 •

edited

Loading

layershifter commented Aug 28, 2016 •

edited

Loading

layershifter commented Oct 6, 2016 •

edited

Loading

levithomason commented Oct 6, 2016 •

edited

Loading

codecov-io commented Oct 10, 2016 •

edited

Loading

levithomason commented Oct 14, 2016 •

edited

Loading

layershifter commented Oct 25, 2016 •

edited

Loading

levithomason Oct 26, 2016 •

edited

Loading

layershifter commented Oct 26, 2016 •

edited

Loading