Skip to content

feat(range): add label prop - #27408

Merged
averyrousseau merged 7 commits into
feature-7.1from
FW-3669
May 10, 2023
Merged

feat(range): add label prop#27408
averyrousseau merged 7 commits into
feature-7.1from
FW-3669

Conversation

@averyrousseau

@averyrousseauaveryrousseau commented May 5, 2023

Copy link
Copy Markdown
Contributor

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

@bolt-new-by-stackblitz

Copy link
Copy Markdown

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

@github-actionsgithub-actionsBot added package: angular @ionic/angular package package: core @ionic/core package package: vue @ionic/vue package labels May 5, 2023
@averyrousseau
averyrousseau marked this pull request as ready for review May 5, 2023 19:35
});
});

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

Copy link
Copy Markdown
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
Copy Markdown
ContributorAuthor

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
Copy Markdown
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.

@sean-perkinssean-perkins left a comment

Copy link
Copy Markdown
Contributor

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.

Comment threadcore/src/components/range/range.tsx Outdated
Comment threadcore/src/components/range/range.tsx Outdated

@sean-perkinssean-perkins left a comment

Copy link
Copy Markdown
Contributor

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.

@liamdebeasiliamdebeasi left a comment

Copy link
Copy Markdown
Contributor

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?

Comment threadcore/src/components/range/range.tsx

@liamdebeasiliamdebeasi left a comment

Copy link
Copy Markdown
Contributor

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?

@liamdebeasiliamdebeasi left a comment

Copy link
Copy Markdown
Contributor

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?

@averyrousseau

Copy link
Copy Markdown
ContributorAuthor

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

Comment threadcore/src/components/range/range.tsx

@liamdebeasiliamdebeasi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Great job!

@averyrousseau
averyrousseau merged commit 368add2 into feature-7.1May 10, 2023
@averyrousseau
averyrousseau deleted the FW-3669 branch May 10, 2023 15:13
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

package: angular@ionic/angular packagepackage: core@ionic/core packagepackage: vue@ionic/vue package

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@averyrousseau@liamdebeasi@thetaPC@sean-perkins@mapsandapps@Ionitron