feat(Confirm): allow custom sizes #2409

ashurbeyli · 2018-01-05T21:32:36Z

ashurbeyli · 2018-01-06T09:41:53Z

There are 2 failed tests that is not related to this issue.

layershifter

Thanks for PR, I've left some comments 👍

layershifter · 2018-01-06T18:37:19Z

docs/app/Examples/addons/Confirm/Variations/ConfirmExampleSize.js

+import React, { Component } from 'react'
+import { Button, Confirm } from 'semantic-ui-react'
+
+class ConfirmExampleSize extends Component {


Let's use a in definition export, i.e. export default class ConfirmExampleSize

layershifter · 2018-01-06T18:40:35Z

docs/app/Examples/addons/Confirm/Variations/ConfirmExampleSize.js

+class ConfirmExampleSize extends Component {
+  state = { open: false }
+
+  show = () => this.setState({ open: true })


Let's add more sense to handler's name: show => handleButtonClick

layershifter · 2018-01-06T18:40:57Z

docs/app/Examples/addons/Confirm/Variations/ConfirmExampleSize.js

+          size="large"
+          header='This is a large confirm'
+          onCancel={this.handleCancel}
+          onConfirm={this.handleConfirm}


Let's sort props in alphabetical order

layershifter · 2018-01-06T18:41:35Z

src/addons/Confirm/Confirm.d.ts

@@ -39,6 +39,9 @@ export interface ConfirmProps extends ModalProps {

  /** Whether or not the modal is visible. */
  open?: boolean;
+
+  /** A modal can vary in size. */


Typo: confirm can vary in size

layershifter · 2018-01-06T18:41:45Z

src/addons/Confirm/Confirm.js

@@ -46,6 +46,9 @@ class Confirm extends Component {

    /** Whether or not the modal is visible. */
    open: PropTypes.bool,
+
+    /** A modal can vary in size */
+    size: PropTypes.oneOf(['fullscreen', 'large', 'mini', 'small', 'tiny']),


Let's also add a small as value to defaultProps

layershifter · 2018-01-06T18:53:44Z

test/specs/addons/Confirm/Confirm-test.js

@@ -40,13 +40,13 @@ describe('Confirm', () => {
    mapValueToProps: content => ({ content }),
  })

-  it('renders a small Modal', () => {
-    wrapperShallow(<Confirm />)
+  it('renders a large Modal', () => {


Let's update this test:

describe('children', () => { it('renders a Modal', () => { shallow(<Confirm />) .type() .should.equal(Modal) }) }) describe('size', () => { it('has "small" size by default', () => { shallow(<Confirm />) .should.have.prop('size', 'small') }) _.forEach(['fullscreen', 'large', 'mini', 'small', 'tiny'], size => { it(`applies "${size}" size', () => { shallow(<Confirm size={size} />) .should.have.prop('size', size) }) }) })

layershifter · 2018-01-06T19:06:34Z

@aabustamante yes, it's a known problem, rerun tests and everything should be fine. There are also lint issues:

Run yarn run lint:fix to fix them 👍

aabustamante

Hi, @layershifter. I am a little bit confused. I think maybe my name was not the one you wanted to mention, anyway I added some comments.

aabustamante · 2018-01-07T01:17:21Z

docs/app/Examples/addons/Confirm/Variations/ConfirmExampleSize.js

+        <Button onClick={this.show}>Show Large</Button>
+        <Confirm
+          open={this.state.open}
+          size="large"


let's keep consistency with single quotes.

Hi, @layershifter. I noticed that there is another file in this same path called ConfirmExampleButtonsupdated a year ago. This file I mention also mixes single quotes and double quotes, I remember that there are some lint tests for this, could this be indicating that these files are not in the coverage of the lint test? or maybe I'm wrong and there is no test for this, if that's the case I think we should create one, what do you think?.

codecov-io · 2018-01-07T11:09:33Z

Codecov Report

Merging #2409 into master will decrease coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2409      +/-   ##
==========================================
- Coverage   99.73%   99.73%   -0.01%     
==========================================
  Files         152      152              
  Lines        2672     2664       -8     
==========================================
- Hits         2665     2657       -8     
  Misses          7        7

Impacted Files	Coverage Δ
src/addons/Confirm/Confirm.js	`100% <ø> (ø)`	⬆️
src/modules/Checkbox/Checkbox.js	`100% <0%> (ø)`	⬆️
src/modules/Progress/Progress.js	`100% <0%> (ø)`	⬆️

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 380cf90...339dd78. Read the comment docs.

levithomason · 2018-01-10T04:43:03Z

src/addons/Confirm/Confirm.js

@@ -46,12 +46,16 @@ class Confirm extends Component {

    /** Whether or not the modal is visible. */
    open: PropTypes.bool,
+
+    /** A modal can vary in size */


This should say Confirm and have a period:

- /** A modal can vary in size */ + /** A Confirm can vary in size. */

Note, I tried to update this but it is on @ashurbeyli's master branch which I cannot push to.

levithomason · 2018-01-10T04:45:41Z

Although we can add this here, all extra Confirm props are passed to the underlying Modal automatically. If we hoist one, we should technically hoist them all. There is an issue for that in #1169.

Again, we can merge this one for now once comments are addressed.

levithomason · 2018-01-10T07:47:34Z

Cool, was just about to make a release. I'll wait for this to pass.

levithomason · 2018-01-10T07:52:52Z

Thank you!

levithomason · 2018-01-10T08:19:59Z

Released in semantic-ui-react@0.77.2

layershifter requested changes Jan 6, 2018

View reviewed changes

layershifter added the needs author feedback label Jan 6, 2018

aabustamante reviewed Jan 7, 2018

View reviewed changes

ashurbeyli force-pushed the master branch from fafda65 to 23de203 Compare January 7, 2018 11:04

layershifter approved these changes Jan 7, 2018

View reviewed changes

layershifter added enhancement ready for review and removed needs author feedback labels Jan 7, 2018

levithomason reviewed Jan 10, 2018

View reviewed changes

levithomason added needs author feedback and removed ready for review labels Jan 10, 2018

feat(Confirm): allow custom sizes

339dd78

ashurbeyli force-pushed the master branch from 23de203 to 339dd78 Compare January 10, 2018 07:43

levithomason added ready for review ready for merge and removed needs author feedback labels Jan 10, 2018

levithomason merged commit 863a94d into Semantic-Org:master Jan 10, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(Confirm): allow custom sizes #2409

feat(Confirm): allow custom sizes #2409

ashurbeyli commented Jan 5, 2018

ashurbeyli commented Jan 6, 2018

layershifter left a comment

layershifter Jan 6, 2018

layershifter Jan 6, 2018

layershifter Jan 6, 2018

layershifter Jan 6, 2018

layershifter Jan 6, 2018

layershifter Jan 6, 2018

layershifter Jan 6, 2018

layershifter commented Jan 6, 2018

aabustamante left a comment

aabustamante Jan 7, 2018

aabustamante Jan 7, 2018

codecov-io commented Jan 7, 2018 •

edited

Loading

levithomason Jan 10, 2018

levithomason Jan 10, 2018

levithomason commented Jan 10, 2018

levithomason commented Jan 10, 2018

levithomason commented Jan 10, 2018

levithomason commented Jan 10, 2018

feat(Confirm): allow custom sizes #2409

feat(Confirm): allow custom sizes #2409

Conversation

ashurbeyli commented Jan 5, 2018

ashurbeyli commented Jan 6, 2018

layershifter 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

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 Jan 6, 2018

aabustamante left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Jan 7, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

levithomason commented Jan 10, 2018

levithomason commented Jan 10, 2018

levithomason commented Jan 10, 2018

levithomason commented Jan 10, 2018

codecov-io commented Jan 7, 2018 •

edited

Loading