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

feat(Modal): impliment Dimmer shorthand #1739

Merged
merged 7 commits into from
Aug 4, 2020

Conversation

qoalu
Copy link
Contributor

@qoalu qoalu commented Jun 7, 2017

Fixes #3195.

This PR adds support for shorthand API in the dimmer prop for Modal. This allows to apply callbacks or props specifically to a dimmer slot:

// Was possible before
<Modal dimmer='blurring' />
<Modal dimmer='inverted' />
// Now it's possible to do
<Modal dimmer={{ blurring: true, id: 'foo', onClick: () => console.log('onDimmerClick') }} />
<div
  class="ui page modals dimmer transition visible active"
  id="foo"
  style="display: flex !important;"
></div>

@codecov-io
Copy link

codecov-io commented Jun 7, 2017

Codecov Report

Merging #1739 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1739      +/-   ##
==========================================
+ Coverage   99.83%   99.83%   +<.01%     
==========================================
  Files         172      173       +1     
  Lines        3101     3105       +4     
==========================================
+ Hits         3096     3100       +4     
  Misses          5        5
Impacted Files Coverage Δ
src/modules/Modal/ModalDimmer.js 100% <100%> (ø)
src/modules/Modal/Modal.js 100% <100%> (ø) ⬆️
src/modules/Modal/ModalHeader.js 100% <100%> (ø) ⬆️

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 02b7b79...2c18613. Read the comment docs.

@levithomason
Copy link
Member

The className prop is already used as the className for the Modal itself, here. I don't think we should apply the className to both the Modal and the Dimmer.

That said, it makes sense to allow the user to control the Dimmer. I'm thinking we should:

  1. Use the actual Dimmer component within the Modal

  2. Explore our shorthand conventions for controlling the Dimmer

    <Modal dimmer={{ className: 'my-dimmer' }} />

@qoalu
Copy link
Contributor Author

qoalu commented Jun 8, 2017

Sure, setting a different prop for Dimmer className is a better solution.

  1. This is a bit counterintuitive because Dimmer actually appears outside of the Modal, not inside it.
  2. That would be the most flexible solution with passing all desired attributes as props to the dimmer Component. My vote for this one.

@layershifter layershifter changed the title Allow setting class on Dimmer when using Modal feat(Modal): impliment Dimmer shorthand Oct 6, 2017
@layershifter
Copy link
Member

Needs #2001.

@stale
Copy link

stale bot commented Jun 14, 2018

There has been no activity in this thread for 90 days. While we care about every issue and we’d love to see this fixed, the core team’s time is limited so we have to focus our attention on the issues that are most pressing. Therefore, we will likely not be able to get to this one.

However, PRs for this issue will of course be accepted and welcome!

If there is no more activity in the next 90 days, this issue will be closed automatically for housekeeping. To prevent this, simply leave a reply here. Thanks!

@stale stale bot added the stale label Jun 14, 2018
@stale
Copy link

stale bot commented Dec 11, 2018

This issue will be closed due to lack of activity for 12 months. If you’d like this to be reopened, just leave a comment; we do monitor them!

@stale stale bot closed this Dec 11, 2018
@layershifter layershifter reopened this Dec 11, 2018
@stale stale bot removed the stale label Dec 11, 2018
@stale
Copy link

stale bot commented Jun 9, 2019

There has been no activity in this thread for 180 days. While we care about every issue and we’d love to see this fixed, the core team’s time is limited so we have to focus our attention on the issues that are most pressing. Therefore, we will likely not be able to get to this one.

However, PRs for this issue will of course be accepted and welcome!

If there is no more activity in the next 180 days, this issue will be closed automatically for housekeeping. To prevent this, simply leave a reply here. Thanks!

@codecov-commenter
Copy link

codecov-commenter commented Jul 31, 2020

Codecov Report

Merging #1739 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1739   +/-   ##
=======================================
  Coverage   99.84%   99.84%           
=======================================
  Files         185      186    +1     
  Lines        3251     3255    +4     
=======================================
+ Hits         3246     3250    +4     
  Misses          5        5           
Impacted Files Coverage Δ
src/modules/Modal/Modal.js 100.00% <100.00%> (ø)
src/modules/Modal/ModalDimmer.js 100.00% <100.00%> (ø)
src/modules/Modal/ModalHeader.js 100.00% <100.00%> (ø)

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 0f9c691...99471f6. Read the comment docs.

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.

Modal: won't put attribute on outermost element
5 participants