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(v5): Drop bsPrefix from FormGroup #5362

Merged
merged 1 commit into from
Aug 5, 2020
Merged

feat(v5): Drop bsPrefix from FormGroup #5362

merged 1 commit into from
Aug 5, 2020

Conversation

kyletsang
Copy link
Member

@kyletsang kyletsang commented Aug 4, 2020

https://v5.getbootstrap.com/docs/5.0/migration/#forms

Dropped .form-group for margin utilities (we’ve replaced our docs examples with .mb-3).

Changes:

  • Removed bsPrefix from FormGroup as the class is no longer supported.
  • Updated all examples with mb-3 to handle spacing

@kyletsang kyletsang added this to In Progress in v5 support via automation Aug 4, 2020
<Form.Group controlId="formBasicEmail">
<Form.Label>Email address</Form.Label>
<Form.Control type="email" placeholder="Enter email" />
<div className="mb-3">
Copy link
Member

Choose a reason for hiding this comment

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

Guess this gives us a good excuse to revive the Layout component idea 😉 Would be fantastic if we could get this implemented for V5

@taion
Copy link
Member

taion commented Aug 4, 2020

wait, hold on – isn't this still useful for DRYing specifying ID between control and label?

@kyletsang
Copy link
Member Author

wait, hold on – isn't this still useful for DRYing specifying ID between control and label?

Could users use the FormContext directly? This FormGroup doesn't do much otherwise.

ie

<FormContext.Provider value={myContext}>
  <Form.Label />
  <Form.Control />
</FormContext.Provider>

@taion
Copy link
Member

taion commented Aug 4, 2020

Probably want to wrap it up a bit better so it just takes the controlId maybe? cc @jquense thoughts?

@bpas247
Copy link
Member

bpas247 commented Aug 4, 2020

Could users use the FormContext directly? This FormGroup doesn't do much otherwise.

But then almost all of our end-users are just going to implement this themselves, so they'd just end up making their own FormGroup components anyways.

If we feel strongly about dropping this component then we should at least add a "recipes" section in our docs so that end-users will know exactly how they should be making that component.

@kyletsang
Copy link
Member Author

Yeah that's true - It's nice to have something that works out of the box. Could we keep this component and remove the bsPrefix? What's the stance on "non-official" components?

@taion
Copy link
Member

taion commented Aug 4, 2020

I think it's a judgment call? This seems in the same category as something like <OverlayTrigger> which is nice/possible given React, but would not be otherwise.

@kyletsang
Copy link
Member Author

Ah ok. In that case, I would vote for just removing bsPrefix if that's fine by everybody.

@taion
Copy link
Member

taion commented Aug 4, 2020

sgtm

@kyletsang
Copy link
Member Author

Thanks for the feedback @taion and @bpas247. Updated PR with changes.

@kyletsang kyletsang changed the title feat(v5): Drop FormGroup feat(v5): Drop bsPrefix from FormGroup Aug 4, 2020
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.

well, in that case i think we'd want to look at generic "layout class" support. idk

@bpas247
Copy link
Member

bpas247 commented Aug 5, 2020

well, in that case i think we'd want to look at generic "layout class" support. idk

yep, having a Layout component would be great since we could handle three major use cases:

  • provide a generic React component that our end users can directly use (instead of just using host elements)
  • unify our as prop APIs everywhere - since any component that uses the as prop can just use the Layout component internally and have that handle the functionality for that
  • improve the TypeScript DX for our end-users that want to utilize any Bootstrap utility classes

Not to mention, this generic layout component API is not anything new in the component library scene (e.g. the Box component from MUI and the Box component from Chakra-UI), so end-users who are switching from other component libraries to ours will already be familiar in some form to the idea behind it.

@bpas247 bpas247 mentioned this pull request Aug 5, 2020
@kyletsang
Copy link
Member Author

I'll merge this now.

I created a TODO note to revisit the margins after the Layout component has been added:
https://github.com/react-bootstrap/react-bootstrap/projects/2#card-43153543

@kyletsang kyletsang merged commit 091b51e into bs5-dev Aug 5, 2020
v5 support automation moved this from In Progress to Done Aug 5, 2020
@kyletsang kyletsang deleted the v5-formgroup branch August 5, 2020 16:48
nikolas added a commit to ccnmtl/mediathread that referenced this pull request Nov 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
v5 support
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants