Skip to content

docs: ui.radio_group#758

Merged
AkshatJawne merged 14 commits into
deephaven:mainfrom
AkshatJawne:765_radio_group_docs
Aug 22, 2024
Merged

docs: ui.radio_group#758
AkshatJawne merged 14 commits into
deephaven:mainfrom
AkshatJawne:765_radio_group_docs

Conversation

@AkshatJawne
Copy link
Copy Markdown
Contributor

@AkshatJawne AkshatJawne commented Aug 20, 2024

Closes #756

@AkshatJawne AkshatJawne self-assigned this Aug 20, 2024
Copy link
Copy Markdown
Contributor Author

@AkshatJawne AkshatJawne left a comment

Choose a reason for hiding this comment

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

Failures here are unlike other PRs, still investigating docstring to determine how to fix failure.

@AkshatJawne
Copy link
Copy Markdown
Contributor Author

Should be good now to review

Comment thread plugins/ui/docs/components/radio_group.md Outdated
Comment thread plugins/ui/docs/components/radio_group.md Outdated
Comment thread plugins/ui/docs/components/radio_group.md Outdated
Comment thread plugins/ui/docs/components/radio_group.md Outdated
AkshatJawne and others added 4 commits August 21, 2024 09:27
Co-authored-by: Don <dsmmcken@gmail.com>
Co-authored-by: Don <dsmmcken@gmail.com>
@AkshatJawne AkshatJawne requested a review from dsmmcken August 21, 2024 17:22
@dsmmcken
Copy link
Copy Markdown
Contributor

I see you added value everywhere. Is value always required? I thought it was optional and used the label? If so, I would show some options like that.

Copy link
Copy Markdown
Contributor Author

@AkshatJawne AkshatJawne left a comment

Choose a reason for hiding this comment

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

@dsmmcken This is interesting, just tried some examples, and it appears that if we do not currently set the value to be the label, if the value is not provided.

The value by default is set to None, so I guess there are two options:

a. Create a ticket to make it so that it sets the value to the label by default, and then include the fact that the value is an optional prop.
b. Do not make any changes to the current logic, and enforce that users have to set value

I am more in favor of A, but open to either

@dsmmcken
Copy link
Copy Markdown
Contributor

I think it should work automatticaly as you describe with option A. I mean that's what you had initially actually tried, so it seems like thats how one would expect it to work. This of course only applies if contents of the label are a string, and not anything more complex (not sure if it accepts icons + text for example, in which case a value would be required.

@mofojed thoughts?

@AkshatJawne
Copy link
Copy Markdown
Contributor Author

Screenshot 2024-08-22 at 9 10 00 AM

It appears that it accepts icons + text, so in cases like this, we would have to have a value set.

So, I think we should create a ticket where we make the value default to the label, if the label is text. Otherwise, if the label is an icon + text for example, we make it so that value has to be passed in.

@dsmmcken @mofojed

@dsmmcken
Copy link
Copy Markdown
Contributor

@AkshatJawne Please ticket this, and also we should make sure checkbox/checbox_group also behaves the same way, as looking at checkbox docs I don't see it talking about the value prop at all.

@AkshatJawne
Copy link
Copy Markdown
Contributor Author

Created ticket: #765

dsmmcken
dsmmcken previously approved these changes Aug 22, 2024
@AkshatJawne AkshatJawne requested a review from dsmmcken August 22, 2024 21:50
@AkshatJawne AkshatJawne merged commit c9b682a into deephaven:main Aug 22, 2024
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.

docs: ui.radio_group

3 participants