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

#3178 - Make the Checkbox/TrueFalse property editor configurable #3179

Merged
merged 8 commits into from Oct 9, 2018

Conversation

Projects
None yet
4 participants
@BatJan
Copy link
Contributor

BatJan commented Oct 5, 2018

Prerequisites

  • I have [https://github.com//issues/3178) for the proposed changes in this PR, the link is: https://github.com//issues/3178
  • I have added steps to test this contribution in the description below

Description

Adding configuration options for the truefalse/checkbox property editor to take advantage of what the umbToggle directive actually offers out of the box. So basically making it possible to tweak it's behavior and appearance based on the options mentioned here https://our.umbraco.com/apidocs/ui/#/api/umbraco.directives.directive:umbToggle

Testing

Now it's possible to

  • Show labels
  • Control the label texts
  • Control the position of the label
  • Hide the icons in the toggle

This is what it looked like before the PR

before-change-checkbox-prevalues

This is what it looks like after the PR
after-change-checkbox-prevalues

As you will probably notice the label is not aligned very well - I decided to fix this in another PR since just removing the class, which is the cause of this might affect other places where the toggle directive is being used. So there is more to it + I'm going to add some focus styling and other small stuff too.

I'll reference the PR for fixing the UI in this issue once I've opened it.

@BatJan BatJan changed the title WIP: #3178 - Make the Checkbox/TrueFalse property editor configurable #3178 - Make the Checkbox/TrueFalse property editor configurable Oct 6, 2018

@bjarnef

This comment has been minimized.

Copy link
Contributor

bjarnef commented Oct 7, 2018

@BatJan any reason we need new prevalue editor for the label position?

src/Umbraco.Web.UI.Client/src/views/propertyeditors/boolean/booleanWithLabels.prevalues.html

Can't it just use the radiobutton list prevalue editor?
If we need a new one it should probably be placed in "prevalues" folder and maybe more a group of toggles (radiobuttonlist). In this case you only have "left" and "right". But it could also be "top", "bottom", "left" and "right" in another cases. Or "left", "center" and "right".

I think it could be a set/group of toggles. Maybe even a select if it should act as a checkbox list (multi choice) or radiobutton list (single choice).

It might be something we could re-use in grid editor styles/settings :)
#1496

@BatJan

This comment has been minimized.

Copy link
Contributor

BatJan commented Oct 7, 2018

@bjarnef Hmm, no seems I have made a brainfart here...I should just extend add some some default values in the boolean view, which will be overwritten if anything has been setup on the scope.model.config - That should be enough really.

I'll change that tomorrow probably :) Good catch!

EDIT: Strike the above! It is needed since I can't specifically customize the appearance of just the one checkbox/boolean for that one particular setting without affecting them all if I just change the boolean directive. Unless I'm missing something obvious I can't see that there is any other way around it? I suppose it would be the same if re-using the radio button list directive. That would need to be a copy as well defining the values - But doing that might actually make a little more sense that using the toggle so one can see, which placement options are available 👍 And yes obviously it should be placed in the prevalues folder instead - But I was certain that I double checked this...But I managed to get it wrong anyway 😄

I have not checked the code but according to the documentation the placement options for the label is currently left or right - From the documentation:

"Sets the label position to the left or right of the switch. It will default to "left" ("left", "right")."

But of course that could be extended I suppose.

@madsrasmussen

This comment has been minimized.

Copy link
Contributor

madsrasmussen commented Oct 8, 2018

Hi Jan, I appreciate all the work put into this PR but I would like to challenge the purpose of these changes before we add more work to the PR.

What are the main thoughts behind adding the extra configurations to the True/False data type?

I know the component supports the settings but to me, this makes a really simple data type harder to use. I don't see the extra benefit added for the extra options. In my opinion, we should find the "best" way to use toggles in the back office and stick with that one. Then we keep everything consistent and we won't experience a lot of different version of the data type.

Please let me know if there are some use cases I have missed.

@bjarnef

This comment has been minimized.

Copy link
Contributor

bjarnef commented Oct 8, 2018

Hi @BatJan and @madsrasmussen

I came by this article/docs about it is okay to use toggles and when checkboxes or radiobuttons should be used. Maybe something we should consider as well, before adding too many toggles ☺️
https://lexicondesign.io/docs/patterns/Forms/radio_check_toggle.html

@madsrasmussen

This comment has been minimized.

Copy link
Contributor

madsrasmussen commented Oct 8, 2018

Great article @bjarnef

Yes, I agree that checkboxes should be used for "selection" scenarios and toggles for "settings" scenarios, so I think it makes sense that the data type is now a toggle.

What I think we need to discuss is how to use the labels and icons with the toggle and then stick with that decision so we don't end up with some property editors with labels, some with icons, etc.

@BatJan

This comment has been minimized.

Copy link
Contributor

BatJan commented Oct 8, 2018

@madsrasmussen Thanks for ciming in :-)

The main thought is that since it's possible to offer these settings why not do it in order for developer to not needing to resort to a 3rd party package to achieve this. The fewer dependencies the better I think.

Sometimes it can be nice to add a textual context to the checkbox whenever it makes sense. For instance the Checkbox/Toggle is often used in different contexts where it might be nice to have the state appear in text as well. So when it's used for showing or hiding a node for navigation it could be nice to have it show a label saying either "Visible in navigation" or "Hidden from navigation" depending on the state. And in another context you might want to hide a certain node from an internal search and then also making use of the toggle with a label saying either "Visible in search" or "Hidden from search" to make a textual support and make it clear that the toggle serves a different purpose. Even though a description text is used the editor might not notice and perhaps they actually see the toggle state and label faster than the title of the field for instance.

So that was my main idea about this - I hope it makes sense? I realise that the better approach before doing the PR with the change would have been to make an issue with a feature suggestion where this could be discussed but sometimes it maybe easier to "Show & Tell" instead. But I will keep this in mind next time I think...."Hey, this is great!" :D

But I can of course also understand your point of view that more configuration can make it harder to use the Checkbox. But I actually considered if it could be worthwhile only having the "Show labels" option appear and if it's set to true then show the label configurations instead making the UI for setting up the datatype less cluttered initially.

On another note - During the work on this I also discovered some minor UI issues, which I fixed in #3187 <-- I still think this PR is relevant even if we end up choosing to discard this one :-)

@BatJan

This comment has been minimized.

Copy link
Contributor

BatJan commented Oct 8, 2018

@bjarnef Just noticed your comment after writing the above - Really nice article indeed.

@madsrasmussen - But that's the thing. I think that for certain scenarios it can be neccessary to do different thing to support the editor. But the solution to that could of course be to install a 3rd party package and keep the core property editor for the checkbox simple.

@madsrasmussen

This comment has been minimized.

Copy link
Contributor

madsrasmussen commented Oct 8, 2018

No No, let's not discard all of it yet 😄I just want us to go through the settings and see if we can make some decisions to have a good consistent user experience.

I am just thinking out loud here:

If we accept that sometimes a label is needed next to the toggle to make it more obvious what the setting does I think there are a couple of things we can do.

We know the toggle is always aligned to the left when used as a property editor so we might be able to only show the label for "On" on the right side. That way we can make sure the toggles always align nicely to the left and look consistent. We could consider removing the setting to show/hide the labels and just show the "On" label if a value is entered?

Regarding the icons, I think we should decide whether they make sense to show or not and then stick to that decision. I don't see why it should be configurable.

@BatJan

This comment has been minimized.

Copy link
Contributor

BatJan commented Oct 8, 2018

@madsrasmussen Haha 😄

I think you're right - We should skip the "show labels" option and make the positon consitent to the right. Looking at the link @bjarnef sent I think it makes really good sense watching the examples they have provided.

So if I understand you correctly this leaves us with 2 additional fields for "on" and "off" labels, which are only being displayed if they have a value skipping the "position", "hide icons" and "show labels" settings. I think that is indeed nicer too.

@BatJan

This comment has been minimized.

Copy link
Contributor

BatJan commented Oct 8, 2018

Good evening folks! 😃

I have made the changes that we have discussed above, which boils it all down to that we only have one additional textfield on the property editor prevalue setup.

The label is now used to decide whether or not a label should be displayed. If the text is empty labels are hidden. If it's not the label is shown.

I re-read your last comments @madsrasmussen and I think that you actually meant we should only have one label, which should be the same no matter if we toggle on or off, which also aligns with the suggestions from the page @bjarnef linked to as well.

The only small downside is that currently the default behavior of the directive is to display "off" if labels are active and no text is provided. So now I just use the labelOn text for the "Off" state too. But perhaps the default behavior should be changed? Or should we keep it as it is? I don't know what areas of the backoffice changing the behavior might have or if it's already being used in some 3rd party packages, which will then need to be modified by the creators too. Looking forward to some opinions on this :-)

This is what the setting up the checkbox looks now
checkbox-prevalues

And this is what it looks like when the label is rendered (As mentioned the UI for aligning the label is fixed in another PR).
checkbox-after-change

This is a lot cleaner indeed 😃

@nul800sebastiaan

This comment has been minimized.

Copy link
Member

nul800sebastiaan commented Oct 9, 2018

Thanks guys, great conversation here!

I've tested all of this and merged in #3187 already.

The functionality works as expected and I tried to do an upgrade from 7.12 to make sure everything gets added properly to the datatype and there's no javascript errors for upgraded toggles.

All good, thanks a lot @BatJan ! 🏅

@nul800sebastiaan nul800sebastiaan merged commit d1b1a1c into umbraco:dev-v7 Oct 9, 2018

1 of 2 checks passed

Cms Continuous #201810080013 failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment