8000 add some classes and ids for components by xvaara · Pull Request #2344 · bootstrap-vue-next/bootstrap-vue-next · GitHub
[go: up one dir, main page]
Skip to content

add some classes and ids for components #2344

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
Merged
merged 1 commit into from
Nov 19, 2024
Merged

add some classes and ids for components #2344

merged 1 commit into from
Nov 19, 2024

Conversation

xvaara
Copy link
Contributor
@xvaara xvaara commented Nov 14, 2024

feat(BDropdown): id for split and menu elements
feat(BFromGroup): add class b-form-group
feat(BImg): add class b-img

Describe the PR

@dwgray @VividLemon think of any other elements that would require a class or id for sub elements?

Small replication

A small replication or video walkthrough can help demonstrate the changes made. This is optional, but can help observe the intended changes. A mentioned issue that contains a replication also works.

PR checklist

What kind of change does this PR introduce? (check at least one)

  • Bugfix 🐛 - fix(...)
  • Feature - feat(...)
  • ARIA accessibility - fix(...)
  • Documentation update - docs(...)
  • Other (please describe)

The PR fulfills these requirements:

  • Pull request title and all commits follow the Conventional Commits convention or has an override in this pull request body This is very important, as the CHANGELOG is generated from these messages, and determines the next version type. Pull requests that do not follow conventional commits or do not have an override will be denied

@xvaara
add some classes and ids for components
bc9b85f 
feat(BDropdown): id for split and menu elements
feat(BFromGroup): add class b-form-group
feat(BImg): add class b-img
@bolt-new-by-stackblitz Bolt.new (by StackBlitz)
Copy link

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@pkg-pr-new pkg.pr.new
Copy link
pkg-pr-new bot commented Nov 14, 2024

Open in Stackblitz

pnpm add https://pkg.pr.new/bootstrap-vue-next/bootstrap-vue-next@2344

pnpm add https://pkg.pr.new/bootstrap-vue-next/bootstrap-vue-next/@bootstrap-vue-next/nuxt@2344

commit: bc9b85f

@xvaara xvaara linked an issue Nov 14, 2024 that may be closed by this pull request
Margin issue with b-form-group in latest update #2316
Closed
6 tasks
@xvaara xvaara mentioned this pull request Nov 14, 2024
Closed
6 tasks
@xvaara
Copy link
Contributor Author
xvaara commented Nov 14, 2024

I added b-prefix since b-avatar etc was using it. Our own utility classes have bv-prefix.

For completeness sake, should we add the b-component-name class to every other component also? or rename our component classes without b-prefix to be inline with bootstraps names?

@dwgray
Copy link
Member
dwgray commented Nov 14, 2024

I added b-prefix since b-avatar etc was using it. Our own utility classes have bv-prefix.

For completeness sake, should we add the b-component-name class to every other component also? or rename our component classes without b-prefix to be inline with bootstraps names?

Thinking this through a bit:

Bootstrap itself doesn't use a namespace prefix, so the pattern that many of our components have by the nature of implementing Bootstrap is that we have a class with a bare name that is basically the name of their 'component' on the main element. Just looking at a few, BAccordion = accordion, BAlert = alert, BBadge = badge, BBreadcrumb = breadcrumb, Button = btn, etc.
So I think we're just looking at the few that don't follow this pattern.
FormGroup is interesting because BS4 had a form-group class and then deprecated the class in BS5.
So I think it would be great to add a class to those that don't in order to fill in the pattern. I agree that it makes sense to use a prefix. And b-* seems fine for this.
I don't think that we should add an additional class to all of the components like the one cited above, since we'd not be getting any functionality. While I'm generally for consistency, adding the verbosity of an additional b-* class for every element that already has a * class seems like overkill. I'd be happy to stick a table in the docs somewhere to make it easy to find the class to add things to for each component (maybe we just add this to the ComponentReference?

Renaming our components to be in line with Bootstrap names—Are you suggesting that we rename Button to Btn? Are there other examples? I'm not sure I have a strong opinion on that.

@VividLemon
Copy link
Member

Renaming our components to be in line with Bootstrap names—Are you suggesting that we rename Button to Btn? Are there other examples? I'm not sure I have a strong opinion on that.

BBtn can be an additional export. Similar to Binput

And b-* seems fine for this

Prefer using this prefix for custom classes

I don't think that we should add an additional class to all of the components like the one cited above, since we'd not be getting any functionality

I'd agree that useless classes like these should be avoided, even ones that are currently included now should probably be removed if they don't do anything

@dwgray
Copy link
Member
dwgray commented Nov 18, 2024

@VividLemon & @xvaara

Okay - I think we have a path forward here. The plan is:

  1. Add b-* to the relatively few components that don't have a bootstrap class already defined
  2. Add aliases for those few components where the bootstrap class is not the same name as t 8000 he component e.g. btn vs. BButton would prompt us to add a BBtn alias.
  3. Document the class name for the 'primary' class for a component, since it may be either b-name or name, which won't be obvious to the developer without either some investigation or seeing it in the documentation.

I would be happy to handle 3.

Since the existing PR conforms to the above guidelines, I'd recommend that we let it through and I can catch the remaining exceptions in my parity pass (and since these are additive, getting every single one isn't urgent, while the ones that @xvaara covered here are cases that have come up - in at least one case, several times).

@xvaara xvaara merged commit 2caf82b into bootstrap-vue-next:main Nov 19, 2024
4 checks passed
@xvaara xvaara deleted the dom-checkup branch November 19, 2024 10:29
xvaara added a commit to xvaara/bootstrap-vue-next that referenced this pull request Nov 19, 2024
@xvaara
Merge remote-tracking branch 'upstream/main'
7dccd61 
* upstream/main: (49 commits)
  fix: rename ref to avoid Vue warnings (Fix bootstrap-vue-next#2337) (bootstrap-vue-next#2353)
  Fix show/hide events and emits (bootstrap-vue-next#1821)
  add some classes and ids for components (bootstrap-vue-next#2344)
  feat: add __usedComponents __usedDirectives property to the BootstrapVueNextResolver function (bootstrap-vue-next#2351)
  doc(BNavbar): Parity pass (bootstrap-vue-next#2347)
  docs: fix lint error (bootstrap-vue-next#2349)
  fix(BNavbarToggle): toggle default slot doesnt have a scoped argument 'expanded' fixes bootstrap-vue-next#2348
  feat!: remove html props -- use equivalent slots fixes bootstrap-vue-next#1930 (bootstrap-vue-next#2311)
  chore: release main (bootstrap-vue-next#2343)
  fix(BDropdown): fix infinite loop on keyboard navigation (bootstrap-vue-next#2342)
  chore: release main (bootstrap-vue-next#2336)
  feat(BPagination): add keyboard shortcuts fixes bootstrap-vue-next#2153
  doc(BNav): Parity pass (bootstrap-vue-next#2332)
  chore: release main (bootstrap-vue-next#2327)
  fix(BTable): dynamic slots not rendering fixes bootstrap-vue-next#2328 (bootstrap-vue-next#2329)
  feat(BTable): make it possible to style custom footers (bootstrap-vue-next#2314)
  chore: release main (bootstrap-vue-next#2323)
  fix(BFormGroup): fix layout problem when label-for is not used (bootstrap-vue-next#2321)
  chore: release main (bootstrap-vue-next#2320)
  feat(BPagination): add small screen support (bootstrap-vue-next#2308)
  ...
@dwgray dwgray mentioned this pull request Dec 3, 2024
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Margin issue with b-form-group in latest update
3 participants
@xvaara @dwgray @VividLemon
0