Skip to content

Conversation

@mshriver
Copy link
Contributor

@mshriver mshriver commented Oct 25, 2022

PF4 has no radio group, just radio

Radio has description/label/checkbox widgets that should be consistent
in practice

body will have to be replaced within a Radio subclass when used in
practice on radios that have additional widgets in their body

Filling a radio just clicks the checkbox, Radio.body will have to be
filled separately from the selection itself.

child_widget_accessed could be used in the body definition, maybe
there's a better strategy to define the body widgets?

OUIA integration will be interesting, as the component type and ID are set on the <input> element, not on the root DIV. Will likely have to play with locator construction.

@mshriver mshriver force-pushed the add-radio branch 2 times, most recently from f5b856f to 9dd0b86 Compare October 25, 2022 17:48
@mshriver mshriver changed the title DRAFT: Add radio widget Add radio widget Oct 25, 2022
@mshriver mshriver marked this pull request as ready for review October 25, 2022 17:49
@mshriver
Copy link
Contributor Author

Would love to hear early thoughts on the Radio design and behavior, planning on including an OUIA class along with it, but that will require a non-trivial class because of the DOM attribute locations.

@mshriver mshriver force-pushed the add-radio branch 2 times, most recently from 3a480f9 to 2987648 Compare October 25, 2022 20:57
PF4 has no radio group, just radio
provide unit tests as well

Radio has description/label/checkbox widgets that should be consistent
in practice

body will have to be replaced within a Radio subclass when used in
practice on radios that have additional widgets in their body

Filling a radio just clicks the checkbox, Radio.body will have to be
filled separately from the selection itself.

child_widget_accessed could be used in the body definition, maybe
there's a better strategy to define the body widgets?
Copy link
Collaborator

@LightOfHeaven1994 LightOfHeaven1994 left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for the effort!

@LightOfHeaven1994 LightOfHeaven1994 merged commit d39ee33 into RedHatQE:master Oct 26, 2022
@digitronik
Copy link
Member

ahhhh I'm late.
LGTM!
I think we need to update the readme as well.

@digitronik
Copy link
Member

Updating for both thanks Mike!

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.

3 participants