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

docs(Modal|Confirm): sort `size` prop in sizes order #3237

Merged
merged 4 commits into from Oct 29, 2018

Conversation

@fcwheat
Copy link
Contributor

fcwheat commented Oct 23, 2018

Simple fix to correct the size property enumeration in the documentation for Modal and Confirm components. Previously the ordering was not uniform (smallest -> largest or largest -> smallest).

Simple fix to correct the size property enumeration in the documentation for Modal and Confirm components. Previously the ordering was not uniform (smallest -> largest or largest -> smallest).
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Oct 23, 2018

Codecov Report

Merging #3237 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3237   +/-   ##
=======================================
  Coverage   99.92%   99.92%           
=======================================
  Files         169      169           
  Lines        2802     2802           
=======================================
  Hits         2800     2800           
  Misses          2        2
Impacted Files Coverage Δ
src/modules/Modal/Modal.js 100% <ø> (ø) ⬆️
src/addons/Confirm/Confirm.js 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 6e83af2...c5bbc7b. Read the comment docs.

@fcwheat

This comment has been minimized.

Copy link
Contributor Author

fcwheat commented Oct 23, 2018

More broadly I am curious why the Modal component doesn't implement the onApprove and onDeny behavior that the Modal element has in the core library (actions close the modal on default unless the user wants to do something else). I see that the Confirm component implements this in a way, but if the Modal had this ability then the Confirm component wouldn't even really need to exist.

@layershifter

This comment has been minimized.

Copy link
Member

layershifter commented Oct 25, 2018

More broadly I am curious why the Modal component doesn't implement the onApprove and onDeny behavior that the Modal element has in the core library

Because there are no real ability to implement this feature for children API without over complications (i.e. Context API) while Shorthand API allows to define callbacks with actions.

@fcwheat

This comment has been minimized.

Copy link
Contributor Author

fcwheat commented Oct 25, 2018

Gotcha, thanks for the feedback.

@layershifter layershifter changed the title docs(Modal/Confirm): Correct size prop enumeration order docs(Modal|Confirm): sort `size` prop in sizes order Oct 29, 2018
@layershifter layershifter merged commit 236bcf8 into Semantic-Org:master Oct 29, 2018
3 checks passed
3 checks passed
ci/circleci Your tests passed on CircleCI!
Details
codecov/patch Coverage not affected when comparing 6e83af2...c5bbc7b
Details
codecov/project 99.92% remains the same compared to 6e83af2
Details
@welcome

This comment has been minimized.

Copy link

welcome bot commented Oct 29, 2018

Congrats on merging your first pull request! 🎉🎉🎉

robot victory dance

@layershifter

This comment has been minimized.

Copy link
Member

layershifter commented Oct 29, 2018

@fcwheat thank you 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.