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

fix(VBtn): accept value prop for underlying DOM element #16465

Merged
merged 1 commit into from
Jan 24, 2023
Merged

fix(VBtn): accept value prop for underlying DOM element #16465

merged 1 commit into from
Jan 24, 2023

Conversation

floroz
Copy link
Contributor

@floroz floroz commented Jan 18, 2023

Description

  • Implements the logic to allow consumers to provide a value prop that will be attached as Node attribute to the underlying DOM element.

Motivation and Context

fix #16188

How Has This Been Tested?

Added Cypress Component testing

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Improvement/refactoring (non-breaking change that doesn't add any features but makes things better)

Checklist:

  • The PR title is no longer than 64 characters.
  • The PR is submitted to the correct branch (master for bug fixes and documentation updates, dev for new features and backwards compatible changes and next for non-backwards compatible changes).
  • My code follows the code style of this project.
  • I've added relevant changes to the documentation (applies to new features and breaking changes in core library)

@floroz
Copy link
Contributor Author

floroz commented Jan 18, 2023

If these changes are fine, I'd be happy to also tackle the group:selected event which seems to have an incorrect signature, since it set to only emit true but never invoked upon click.

I assume the correct behaviour should be to emit the props.value ad added in the onClick

NVM, just learned about undocumented getCurrentInstance Vue API :)

@nekosaur
Copy link
Member

If these changes are fine, I'd be happy to also tackle the group:selected event which seems to have an incorrect signature, since it set to only emit true but never invoked upon click.

I assume the correct behaviour should be to emit the props.value ad added in the onClick

That emit declaration is correct iirc. It is emitted from the useGroupItem composable

@floroz floroz requested review from nekosaur and KaelWD and removed request for nekosaur and KaelWD January 18, 2023 15:48
@floroz floroz requested review from KaelWD and removed request for nekosaur January 19, 2023 09:02
@johnleider johnleider added T: bug Functionality that does not work as intended/expected C: VBtn labels Jan 23, 2023
@johnleider johnleider added this to the v3.x.x milestone Jan 23, 2023
@johnleider johnleider changed the title fix(vbtn): VBtn accepts a value prop to be attached to the underlying DOM element fix(VBtn): accept value prop for underlying DOM element Jan 23, 2023
@floroz floroz requested review from johnleider and removed request for KaelWD January 24, 2023 12:44
@floroz
Copy link
Contributor Author

floroz commented Jan 24, 2023

Thanks for the detailed feedback @johnleider.

I've polished the PR and implemented your suggestions.

@floroz floroz requested a review from johnleider January 24, 2023 20:07
restore the logic for the value attribute to receive any type of prop and convert them to string to
pass to the underlying DOM element

fix #16188
@floroz floroz requested a review from johnleider January 24, 2023 20:16
@johnleider johnleider modified the milestones: v3.x.x, v3.1.x Jan 24, 2023
@johnleider johnleider merged commit 09bc5dc into vuetifyjs:next Jan 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: VBtn T: bug Functionality that does not work as intended/expected
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants