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(range): add label prop #27408

Merged
merged 7 commits into from
May 10, 2023
Merged

feat(range): add label prop #27408

merged 7 commits into from
May 10, 2023

Conversation

averyjohnston
Copy link
Contributor

@averyjohnston averyjohnston commented May 5, 2023

Issue number: N/A


What is the current behavior?

Labels on ion-range can only be set via the label slot. When only plain text is needed, this is cumbersome because you need to add an entire new element to wrap the label.

What is the new behavior?

Label prop added. If both the prop and slot are used, the prop will take priority.

Does this introduce a breaking change?

  • Yes
  • No

Other information

@stackblitz
Copy link

stackblitz bot commented May 5, 2023

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

@github-actions github-actions bot added package: angular @ionic/angular package package: core @ionic/core package package: vue @ionic/vue package labels May 5, 2023
@averyjohnston averyjohnston marked this pull request as ready for review May 5, 2023 19:35
@@ -125,6 +125,32 @@ configs().forEach(({ title, screenshot, config }) => {
expect(await range.screenshot()).toMatchSnapshot(screenshot(`range-items-fixed`));
});
});

test.describe('range: label prop', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be limited to only one mode and one direction? The label prop doesn't change functionality regardless of mode/direction.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I intentionally left all the configs in because the prop needed some styling separate from the slot, so I figured it would be good to verify no visual regressions in all cases, but I could go either way on it, honestly.

Copy link
Contributor

Choose a reason for hiding this comment

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

If the majority is okay as is, then I can be on board.

Copy link
Contributor

@sean-perkins sean-perkins left a comment

Choose a reason for hiding this comment

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

Can we add an additional test for when using both a label property and label slot? This can likely be a unit test, just to query for the expected DOM node.

core/src/components/range/range.tsx Outdated Show resolved Hide resolved
core/src/components/range/range.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@sean-perkins sean-perkins left a comment

Choose a reason for hiding this comment

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

Can we add an additional test for when using both a label property and label slot? This can likely be a unit test, just to query for the expected DOM node.

Copy link
Contributor

@liamdebeasi liamdebeasi left a comment

Choose a reason for hiding this comment

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

Can we add a usage of the label prop to the a11y file so we can run axe checks?

core/src/components/range/range.tsx Show resolved Hide resolved
Copy link
Contributor

@liamdebeasi liamdebeasi left a comment

Choose a reason for hiding this comment

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

Can we add a usage of the label prop to the a11y file so we can run axe checks?

Copy link
Contributor

@liamdebeasi liamdebeasi left a comment

Choose a reason for hiding this comment

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

Is there a docs PR associated with this?

@averyjohnston
Copy link
Contributor Author

Docs PR created: ionic-team/ionic-docs#2955

Copy link
Contributor

@liamdebeasi liamdebeasi left a comment

Choose a reason for hiding this comment

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

Great job!

@averyjohnston averyjohnston merged commit 368add2 into feature-7.1 May 10, 2023
@averyjohnston averyjohnston deleted the FW-3669 branch May 10, 2023 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: angular @ionic/angular package package: core @ionic/core package package: vue @ionic/vue package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants