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.

Sign up for GitHub

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

Jump to bottom

Add size prop for FormLabel component - Fixes #4886 #4893

Merged
merged 5 commits into from
Jan 15, 2020
Merged

Add size prop for FormLabel component - Fixes #4886 #4893

merged 5 commits into from
Jan 15, 2020

Conversation

raghavi92
Copy link
Contributor

No description provided.

taion
taion reviewed Dec 13, 2019
View reviewed changes
Copy link
Member

@taion taion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fix the lint errors and also update the tests.

src/FormLabel.js Outdated
*
* @type {('sm'|'lg')}
*/
size: PropTypes.string,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be merged with the column prop.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as in column should take true or "sm" or "lg"

Copy link
Contributor Author

@raghavi92 raghavi92 Dec 13, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. But I thought it would be more consistent with FormControl this way?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the issue is that the prop doesn't do anything if you're not specifying column...

@raghavi92
Copy link
Contributor Author

Can you review this @taion ?

taion
taion reviewed Dec 14, 2019
View reviewed changes
www/src/examples/Form/InputSizes.js Outdated
@@ -1,7 +1,14 @@
<>
<Form.Group>
<Form.Label size="lg" col>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you fix this to show the actual API, and merge this in with the horizontal form groups section, or else make a new section paralleling the upstream TWBS docs?

src/FormLabel.js Outdated Show resolved Hide resolved
@kosmiq
Copy link
Collaborator

kosmiq commented Jan 9, 2020 •
edited
Loading

Checked into this. Looking on regular bootstrap, looking at this code and what @raghavi92 wrote in his issue I have a few thoughts.

They are separate layouts within the same component in bootstrap (and uses different classes as well).

So to cover the OP original request, as I have read it and after looking at Bootstrap 4.4, the Form.Label should have the size prop for specifying size when it is used in a vertial layout and use the column prop for sizing when in a horizontal layout (as the column prop currently does).

Actually scratch that. I just realized that Bootstrap does not have classes for sizing labels when they are not in a column layout.

@taion taion requested a review from jquense January 15, 2020 03:59
@jquense jquense merged commit 0ae281c into react-bootstrap:master Jan 15, 2020
@jquense
Copy link
Member

jquense commented Jan 15, 2020

thanks!

@raghavi92
Copy link
Contributor Author

Thank you!

adamayres added a commit to lithiumtech/react-bootstrap that referenced this pull request Feb 5, 2020
@adamayres
Merge branch 'master' into classNameMapSupport
43b051c 
* master: (54 commits)
  docs: Add Glyphicon removal to "Migrating to v1" doc (react-bootstrap#4967)
  chore(deps): update all non-major dependencies (react-bootstrap#4964)
  docs: fixed typos (react-bootstrap#4957)
  refactor(OverlayTrigger): convert to function component (react-bootstrap#4938)
  Add missing horizontal in ListGroupProps (react-bootstrap#4864)
  chore(deps): update all non-major dependencies (react-bootstrap#4953)
  chore(deps): update dependency cross-env to v7 (react-bootstrap#4949)
  Label large dropdown as large in playground (react-bootstrap#4945)
  Fix typographical errors in Button docs (react-bootstrap#4943)
  docs: fix dropdown typos (react-bootstrap#4944)
  docs: Fix component spacing in examples (react-bootstrap#4914)
  chore(deps): update all non-major dependencies (react-bootstrap#4941)
  chore(deps): update dependency lint-staged to v10 (react-bootstrap#4942)
  react-bootstrap#4884 add responsive containers (react-bootstrap#4902)
  feat: add size prop for FormLabel component - Fixes react-bootstrap#4886 (react-bootstrap#4893)
  tests: added for Modal and OverlayTrigger (react-bootstrap#4931)
  test: increase test coverage with minor refactor (react-bootstrap#4922)
  chore(deps): update all non-major dependencies (react-bootstrap#4929)
  docs: fix element type used for Breadcrumb (react-bootstrap#4926)
  chore(deps): update dependency husky to v4 (react-bootstrap#4923)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jquense jquense jquense approved these changes

@taion taion taion approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@raghavi92 @kosmiq @jquense @taion @raghavi-k