Skip to content

feat(BDropdown): add variant, classes and correct attrs to text sub components #2258

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
xvaara merged 7 commits into from
Oct 13, 2024
Merged

feat(BDropdown): add variant, classes and correct attrs to text sub components #2258

xvaara merged 7 commits into from
Oct 13, 2024

Conversation

@xvaara
Copy link
Contributor

@xvaara xvaara commented Oct 10, 2024

Describe the PR

closes #2244

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

@bolt-new-by-stackblitz
Copy link

bolt-new-by-stackblitz bot commented Oct 10, 2024

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

@pkg-pr-new
Copy link

pkg-pr-new bot commented Oct 10, 2024 •
edited
Loading

Open in Stackblitz

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

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

commit: 0201135

@dwgray
Copy link
Member

dwgray commented Oct 11, 2024

In BSV, the innerClass, when implemented, was more specific - e.g. formClass, textClass, etc. I could argue either way for doing that vs. using the more generic term in all cases. I'm happy whichever way you want to go. But I do strongly prefer that we maintain internal consistency. If you want to make everything innerClass, that would require changing basically all the dropdown sub-components you didn't touch here from their specific names to innerClass, or to go the other direction use the same prop names as BSV used. @VividLemon , do you have any thoughts on this?

@xvaara
Copy link
Contributor Author

xvaara commented Oct 11, 2024

@dwgray you are right, I was originally thinking that the components are almost identical so could we just have BDropdownComponent whit a prop to change it to form/divider/text/header

@dwgray
Copy link
Member

dwgray commented Oct 12, 2024

@xvaara I believe that we should implement novalidate and validated on BDropdownForm - these are implemented on BForm and BNavForm, so it would be weird not to have them on BDropdownForm. I've got a commit with these changes 7b51923 - I don't see a way to apply my commit to your PR, but if you can do it or point me at how to do it that would be great.

I'd like to mark disabled on BDropdownForm as deprecated - in BSV, this appears to disable all of the controls on the form. This is not implemented on BForm or BNavForm in BSV. This is certainly something that could be added after a v1 in a non-breaking way if someone feels it's necessary.

@xvaara
Copy link
Contributor Author

xvaara commented Oct 12, 2024

@dwgray I added the stuff from that commit to this.

dwgray
dwgray approved these changes Oct 13, 2024
View reviewed changes
Copy link
Member

@dwgray dwgray left a comment

Choose a reason for hiding this comment

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

LGTM with the exception of the duplicate test.

@xvaara xvaara merged commit d2d4905 into bootstrap-vue-next:main Oct 13, 2024
@xvaara xvaara deleted the dropdown-subcomponents branch October 13, 2024 00:18
@github-actions github-actions bot mentioned this pull request Oct 12, 2024
Merged
@dwgray dwgray mentioned this pull request Oct 13, 2024
Merged
6 tasks
xvaara added a commit to xvaara/bootstrap-vue-next that referenced this pull request Oct 16, 2024
@xvaara
Merge remote-tracking branch 'upstream/main'
b88d260 
* upstream/main:
  chore: release main (bootstrap-vue-next#2275)
  fix(BFormCheckbox): indeterminate state not working properly fixes bootstrap-vue-next#2271 (bootstrap-vue-next#2274)
  chore: release main (bootstrap-vue-next#2270)
  fix(BModal): fix backdrop when modal is shown using v-if (bootstrap-vue-next#2269)
  chore: release main (bootstrap-vue-next#2259)
  fix(BFormInput): bad model modifier trim behavior fixes bootstrap-vue-next#2253 (bootstrap-vue-next#2267)
  fix(BModal): multiple modals backdrop fix (bootstrap-vue-next#2263)
  feat: clean deprecated classes and props from bootstrap (sr-only => visually-hidden)
  docs(BDropdown): Fill out component references for subcomponents (bootstrap-vue-next#2265)
  feat(BDropdown): add variant, classes and correct attrs to text sub components (bootstrap-vue-next#2258)
  fix(BModal): fix backdrop click prevention, fix flickering when no-fade (bootstrap-vue-next#2262)
  fix(BPopover): calculate mouse and element positions in a performant way (bootstrap-vue-next#2252)
xvaara added a commit to xvaara/bootstrap-vue-next that referenced this pull request Oct 21, 2024
@xvaara
Merge remote-tracking branch 'upstream/main' into fix-show-hide-events
653829b 
* upstream/main:
  test: form checkbox indeterminate behavior (bootstrap-vue-next#2279)
  chore: release main (bootstrap-vue-next#2278)
  fix(BModal): remove scrolllock on unmount (bootstrap-vue-next#2277)
  chore: release main (bootstrap-vue-next#2275)
  fix(BFormCheckbox): indeterminate state not working properly fixes bootstrap-vue-next#2271 (bootstrap-vue-next#2274)
  chore: release main (bootstrap-vue-next#2270)
  fix(BModal): fix backdrop when modal is shown using v-if (bootstrap-vue-next#2269)
  chore: release main (bootstrap-vue-next#2259)
  fix(BFormInput): bad model modifier trim behavior fixes bootstrap-vue-next#2253 (bootstrap-vue-next#2267)
  fix(BModal): multiple modals backdrop fix (bootstrap-vue-next#2263)
  feat: clean deprecated classes and props from bootstrap (sr-only => visually-hidden)
  docs(BDropdown): Fill out component references for subcomponents (bootstrap-vue-next#2265)
  feat(BDropdown): add variant, classes and correct attrs to text sub components (bootstrap-vue-next#2258)
  fix(BModal): fix backdrop click prevention, fix flickering when no-fade (bootstrap-vue-next#2262)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@VividLemon VividLemon VividLemon requested changes

@dwgray dwgray dwgray 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.

Several of the sub-components of BDropdown are not fully implemented

3 participants

@xvaara @dwgray @VividLemon