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(ui5-switch): add required property #7324

Merged
merged 11 commits into from
Jul 26, 2023
Merged

feat(ui5-switch): add required property #7324

merged 11 commits into from
Jul 26, 2023

Conversation

hinzzx
Copy link
Contributor

@hinzzx hinzzx commented Jul 12, 2023

Introducing the required property for the Switch control.
We now also enable the Form Support feature within the control, which means now the Switch component, could be easily used and integrated within forms as well.

Fixes: #5897

@hinzzx hinzzx requested a review from ilhan007 July 14, 2023 14:51
@nnaydenow
Copy link
Contributor

Please also check what would be the bevahiour of the newly added property inside following scenario: #7319 (please rework it to work in your case by replacing ui5-checkbox with ui5-switch)

Copy link
Contributor

@unazko unazko left a comment

Choose a reason for hiding this comment

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

According to the screen reader announcements and attributes all seems to be working fine.
About the form support I'll check as well.

packages/main/test/pages/Switch.html Outdated Show resolved Hide resolved
packages/main/test/pages/Switch.html Outdated Show resolved Hide resolved
packages/playground/_stories/main/Switch/Switch.stories.ts Outdated Show resolved Hide resolved
@hinzzx hinzzx requested review from nnaydenow and unazko July 18, 2023 06:22
Copy link
Contributor

@unazko unazko left a comment

Choose a reason for hiding this comment

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

I'm fine with the change.

Copy link
Contributor

@nnaydenow nnaydenow left a comment

Choose a reason for hiding this comment

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

Add test test validates the scenario in a form. You can use html like:

	<form id="switchForm">
		<ui5-switch id="switch1" checked></ui5-switch>
		<ui5-switch id="switch2" required></ui5-switch>
		<br><br>
		<ui5-button id="switchSubmit" type="Submit">Submit</ui5-button>
	</form>

And you can check how formElement.checkValidity() method works and maybe you can add it into the test

@hinzzx hinzzx requested a review from nnaydenow July 24, 2023 09:16
@hinzzx hinzzx requested a review from nnaydenow July 25, 2023 14:45
@hinzzx hinzzx closed this Jul 26, 2023
@hinzzx hinzzx reopened this Jul 26, 2023
@hinzzx hinzzx merged commit 0a01918 into main Jul 26, 2023
8 of 9 checks passed
@hinzzx hinzzx deleted the switch-required branch July 26, 2023 11:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Switch: add required
4 participants