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 dynamic buttons #254

Merged

Conversation

justparking
Copy link
Contributor

Covers issue /issues/252.

@mcartmel - please review naming, etc.

@scroix - do you have any opinion on the naming given it's based on the <dynamicselect> element; <dynamicselectbuttons> has been chosen for now.

I was thinking maybe just <dynamicbuttons> or <dynamicbuttongroup>?

@scroix
Copy link
Member

scroix commented Jul 18, 2022

I tried having a go documenting the types back in the Mk1 recipe.
https://github.com/museumsvictoria/nodel-recipes/tree/master/Frontend/Mk1
(There are some screenshots there as well)

Back then, the names were:

  • DynamicSelect
  • Select
  • PartialSwitch
  • Switch
  • Pill
  • Range
  • Button

I'd happily support anything which remained sensibly consistent. Sticking "Dynamic" at the front of an otherwise static widget-type seems reasonable to me.

As for the suffix. It looks like /issues/252 is most similar to Button from Mk1. So, I would say, DynamicButton, not unlike Select -> DynamicSelect.

edit* DynamicButtonGroup is even better.

Here is a static ButtonGroup.

Screen Shot 2022-07-18 at 7 16 16 pm

<buttongroup>
<button action='PresetChromecast' class='btn-default'>Chromecast</button>
<button action='PresetTV' class='btn-default'>TV</button>
<button action='PresetButler' class='btn-default'>Signage</button>
<button action='PresetSpare1' class='btn-default'>HDMI 1<badge event="Status2"/></button>
<button action='PresetSpare2' class='btn-default'>HDMI 2<badge event="Status2"/></button>
</buttongroup>

@mcartmel
Copy link
Contributor

Late to the party here I know, sorry.

The other option is to add a type attribute, e.g. <dynamicselect type="buttons"/>. It's a convention already since we have <buttongroup type="vertical"/>.

@justparking
Copy link
Contributor Author

Hey @morimoriysmoon,

Yeah, I've been thinking that I'm not comfortable with the name<dynamicselectbuttons> ❌.

<dynamicbuttongroup>✅ better indicates it's a dynamic form of a button group.

In this UI framework the select refers to the appearance of a selection box which is not relevant here.

dynamic is the key concept we're adding here.

This is all wishy-washy but <dynamicbuttongroup> does seems the clearest to me.

Can you please update the code accordingly and let's finally put this to bed.

@justparking justparking merged commit f1f5f4c into museumsvictoria:master Nov 21, 2022
@justparking justparking deleted the feat-dynamic-buttons branch November 21, 2022 23:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants