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(NcActionButton): support boolean value for radio type #5134

Merged
merged 1 commit into from Jan 26, 2024

Conversation

ShGKme
Copy link
Contributor

@ShGKme ShGKme commented Jan 25, 2024

🏃‍♂️ Short description

Allows to have checkbox-like usage for developers having radio semantics for users.

☑️ Resolves

Currently NcActionButton[type="radio"] supports only string modelValue based on value value. For example, if we have alignment, we can use the button with one string variable with left | center | right value.

<NcActionButton :model-value.sync="align" value="left"   type="radio" />
<NcActionButton :model-value.sync="align" value="center" type="radio" />
<NcActionButton :model-value.sync="align" value="right"  type="radio" />

align = left | center | right

It is great in a general case. But sometimes it is more convenient to define boolean check value for each button. For example, in text, where the value is more complex like ['heading', { level: 1 }] and is managed by Tiptop.

Imagine, we have such a data structure and update method. So we want to define the checked state for each button as it is, not by value.

const align = {
  isLeft: false,
  isCenter: true,
  isRight: false,
}

function setAlign(direction) {
  this.align.isLeft = false
  this.align.isCenter = false
  this.align.isRight = false
  this.align[`is${direction}`] = true
},
<NcActionButton :model-value="align.isLeft"   type="radio" @update:modelValue="setAlign('Left')" />
<NcActionButton :model-value="align.isCenter" type="radio" @update:modelValue="setAlign('Center')" />
<NcActionButton :model-value="align.isRight"  type="radio" @update:modelValue="setAlign('Right')" />

This PR introduces such a feature, based on modelValue type.

🖼️ Screenshots

nc-action-button-radio-boolean

🏁 Checklist

  • ⛑️ Tests are included or are not applicable
  • 📘 Component documentation has been extended, updated or is not applicable

@ShGKme ShGKme added enhancement New feature or request 2. developing Work in progress feature: actions Related to the actions components labels Jan 25, 2024
@ShGKme ShGKme added this to the 8.6.0 milestone Jan 25, 2024
@ShGKme ShGKme requested a review from susnux January 25, 2024 03:22
@ShGKme ShGKme self-assigned this Jan 25, 2024
@ShGKme
Copy link
Contributor Author

ShGKme commented Jan 25, 2024

@juliushaertl Am I right that in text with Tiptop it is easier to detect if a button is active like for ['heading', { level: 1 }], than to have a string value for each option?

@ShGKme ShGKme added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Jan 25, 2024
@juliushaertl
Copy link
Contributor

That sounds like a sane addition. One thing I'm unsure about is switching to radio buttons in text give then current behaviour that you can toggle for example a heading style by clicking again on it. That doesn't seem like something a radio button would usually handle (unless there is an additional radio state for the paragraph style for example)

@ShGKme
Copy link
Contributor Author

ShGKme commented Jan 26, 2024

One thing I'm unsure about is switching to radio buttons in text give then current behaviour that you can toggle for example a heading style by clicking again on it. That doesn't seem like something a radio button would usually handle (unless there is an additional radio state for the paragraph style for example)

I'm not sure, I understand where I can find this behavior...

When I click on the "Heading" menu button, it always only opens the menu and doesn't change the styles, until I select the heading level.

And for disabling heading, I need to click on the selected heading.

Header

But I realised now that click on selected radio button should probably disable the the selected item

Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
@ShGKme ShGKme force-pushed the feat/nc-action-button--boolean-radio branch from e970ffc to 68fe201 Compare January 26, 2024 09:43
@ShGKme
Copy link
Contributor Author

ShGKme commented Jan 26, 2024

Added toggle behavior for boolean values

action-radio-toggle

@juliushaertl
Copy link
Contributor

Yes, that is what I was referring to, toggling the radio state on the actual menu entries.

Copy link
Contributor

@susnux susnux left a comment

Choose a reason for hiding this comment

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

Might lead to situations where multiple radio buttons are active on the same time, but thats even possible now. So good for me, I like the feature!

@ShGKme
Copy link
Contributor Author

ShGKme commented Jan 26, 2024

Might lead to situations where multiple radio buttons are active on the same time

Yes, but the same with native input[type=radio], if they have duplicated name or no name.

We probably may add some automatic grouping in future:

  • Easy in NcActionsButtonGroup
  • A bit complex in NcActions where many groups separated by NcActionSeparator may exist.

@ShGKme ShGKme merged commit 7780de9 into master Jan 26, 2024
15 checks passed
@ShGKme ShGKme deleted the feat/nc-action-button--boolean-radio branch January 26, 2024 19:44
susnux added a commit that referenced this pull request Jan 26, 2024
…ion-button--boolean-radio"

This reverts commit 7780de9, reversing
changes made to 612385f.
@susnux susnux restored the feat/nc-action-button--boolean-radio branch January 26, 2024 23:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews enhancement New feature or request feature: actions Related to the actions components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants