Readonly boolean #2747

Kiarokh · 2024-01-30T14:27:41Z

fix https://github.com/Lundalogik/crm-feature/issues/3312

instead of https://github.com/Lundalogik/lime-crm-building-blocks/pull/319

to test in Lime Admin, add this example config on a boolean field

{
	"readonlyLabels": [
		{
			"value": true,
			"icon": "news",
			"text": "Is subscribed to receive newsletters"
		},
		{
			"value": false,
			"icon": {
				"name": "cancel_subscription",
				"color": "rgb(var(--color-orange-default))"
			},
			"text": "Is unsubscribed from newsletters"
		}
	]
}

Review:

Commits are atomic
Commits have the correct type for the changes made
Commits with breaking changes are marked as such

Browsers tested:

(Check any that applies, it's ok to leave boxes unchecked if testing something didn't seem relevant.)

Windows:

Chrome
Edge
Firefox

Linux:

Chrome
Firefox

macOS:

Chrome
Firefox
Safari

Mobile:

Chrome on Android
iOS

github-actions · 2024-01-31T09:28:40Z

Documentation has been published to https://lundalogik.github.io/lime-elements/versions/PR-2747/

src/components/checkbox/checkbox.scss

Kiarokh · 2024-02-04T10:15:11Z

src/components/checkbox/checkbox.tsx

+    /**
+     * The label to show, when the component is `readonly` and its `value` is `true`.
+     * This can be used to clarify what kind of data is being visualized.
+     * If not set, the `label` property will be used.
+     */
+    @Prop({ reflect: true })
+    public readonlyTrueLabel?: string;
+
+    /**
+     * The label to show, when the component is `readonly` and its `value` is `false`.
+     * This can be used to clarify what kind of data is being visualized.
+     * If not set, the `label` property will be used.
+     */
+    @Prop({ reflect: true })
+    public readonlyFalseLabel?: string;
+
+    /**
+     * The icon to show, when the component is `readonly` and its `value` is `true`.
+     * This can be used to clarify what kind of data is being visualized.
+     * If not set, a default icon with a default color will be used.
+     * Colors can be customized using the `Icon` interface.
+     */
+    @Prop({ reflect: true })
+    public readonlyTrueIcon?: string | Icon;
+
+    /**
+     * The icon to show, when the component is `readonly` and its `value` is `false`.
+     * This can be used to clarify what kind of data is being visualized.
+     * If not set, a default icon with a default color will be used.
+     * Colors can be customized using the `Icon` interface.
+     */
+    @Prop({ reflect: true })
+    public readonlyFalseIcon?: string | Icon;
+


A config like this:

{ "readonlyProps" = { "trueLabel": "Still works here", "falseLabel": "Has quit their job", "trueIcon": { "name": "inactive_state", "color": "rgb(var(--color-gray-default))" }, "falseIcon": { "name": "in_progress", "color": "rgb(var(--color-sky-default))" } } }

Doesn't do anything in Lime Admin at all.

I think these new props should somehow be added to the extraProps of the checkbox widget 👇 or somewhere in this file

lime-elements/src/components/form/widgets/checkbox.ts

Line 20 in 0f56884

extraProps: {

Other relevant and similar issues:

It's also worth nothing that adding help which we did here: feat(form): enable providing rich help next to form fields #2622, is not possible to do via Lime Admin on any component, but writing it in the schema in the code works:

Adding helperText on the boolean field is not possible via Lime Admin.

Adding styles (such as CSS variables of components) is not possible on any field via Lime Admin.

I think same things are wrong with out implementations about these.

Maybe you guys can help @adrianschmidt @vicgeralds @jgroth

There's an issue about the CSS variables here Lundalogik/crm-feature#3772
I thought it was possible to specify props for the components in lime-admin, but maybe we need to fix some bug there as well 🤔

It is possible to specify props for the component, and it also works (at least for text fields) without specifying a component name.

The problem with help is that it's not a property to pass. It's only a feature of limel-form when (ab)using schemas. 😉

Adding helperText on the boolean field is not possible via Lime Admin.

This should work, but I can see that it doesn't when trying it out. Not sure yet why 😕

The problem with help is that it's not a property to pass. It's only a feature of limel-form when (ab)using schemas. 😉

😮 Then we completely missed the target @civing @john-traas . One of the most important reasons for adding this was to be able to add contextual help to form elements, both on cards (by consultants) and in admin (link to platform docs by us).

The reason specifying props for a boolean field didn't work, is that we don't pass a default custom component name, which we do apparently for text fields, for example. So it works if you specify limel-checkbox as component name.

The reason specifying props for a boolean field didn't work, is that we don't pass a default custom component name, which we do apparently for text fields, for example. So it works if you specify limel-checkbox as component name.

The problem with that is that limel-checkbox does not have a value prop (it has checked instead 😞 )

The reason limel-checkbox has checked instead of value is that that's what a native html checkbox has, and my idea originally was to keep Lime Elements' components as close to drop-in replacements as possible.

It feels like that was less possible than I originally intended, and I think we have already more or less decided to move away from that idea.

Adding a value property that takes precedence over checked ought to be simple, and I would suggest that we deprecate checked at the same time, if you all agree.

Ping @Lundalogik/lime-elements-maintainers.

😮 Then we completely missed the target @civing @john-traas . One of the most important reasons for adding this was to be able to add contextual help to form elements, both on cards (by consultants) and in admin (link to platform docs by us).

Right now this is a feature of lime-forms where you specify the inputs for the help component in the schema, yes.

If we would like to make this generic for all components, we need to either support it as props for every component or do some other clever shenanigans. I really dislike the idea of violating DRY by extending every component with the same props.

Anyhow, we have the helper component so we can in theory add it somehow to components outside of lime-form.

src/components/checkbox/checkbox.tsx

Kiarokh · 2024-02-06T09:08:28Z

src/components/switch/switch.tsx

+        if (this.readonly) {
+            return [
+                <limel-readonly-boolean


by the way @jgroth, I don't fully buy the argument that the consumer who has control over the code can also create a separate design for the readonly mode. Of course your argument is 100% valid, but with the same argument we could say that the consumer can just use a native HTML checkbox, or input field, popover, or many of the things that this component library offers.

One of the strong points of having a UI component library is that it helps keeping the UI consistent and clean, and making it easy for a developer to make a decent UI, without taking too many layout and ui decisions about minute graphical details.

If everyone was forced to create their own stuff for everything, then the products using this library would become messy and inconsistent.

Also, if A: the readonly is a feature of our boolean field, and B: the boolean fields create this classic usability issue and lead to confusions, then it make a lot of sense that the component itself offers a solution that prevent the usability issues. We can't expect the consumer to come up with solutions. If they could, we didn't have to make this PR neither.

by the way @jgroth, I don't fully buy the argument that the consumer who has control over the code can also create a separate design for the readonly mode.

They don't have to create a separate design for it. There's already a new component added in this PR that could be used (if it's made public). That also opens the possibility to use it in more scenarios than for only boolean fields. Perhaps you want this design as well if you have a readonly option field?

One of the strong points of having a UI component library is that it helps keeping the UI consistent and clean, and making it easy for a developer to make a decent UI, without taking too many layout and ui decisions about minute graphical details.

Yes, I agree, but that does not mean that every component needs to be handle every kind of scenario. Components should ideally be small and have a single purpose, and if you need additional functionality you should create compositions of different components. This is the same problem as with the help property that you described above. If we need to add these kinds of properties to all the components we are bloating them and not keeping them simple.

If everyone was forced to create their own stuff for everything, then the products using this library would become messy and inconsistent.

I'm not sure what you mean by this, but if the new component is public no one has to create their own component. We can still add functionality to lime-admin to let this be configurable without consumers creating their own components.

Perhaps a good compromise would be to:

not include this readonly mode in limel-checkbox

make limel-readonly-bool public

create a component in lime-crm-building-blocks that combines the two

Why the component in building-blocks? Because we want it to be easy to use this composite component in the forms in Lime CRM, without having to create a custom form component (and thereby requiring isolated cloud). I assume we even want to default our boolean fields to this "composite behaviour"?

They don't have to create a separate design for it. There's already a new component added in this PR that could be used (if it's made public). That also opens the possibility to use it in more scenarios than for only boolean fields. Perhaps you want this design as well if you have a readonly option field?

I have no objection if this component is public. But I don't think its usage applies to other components in Lime Elements, than our own boolean fields. The issue it is addressing is about the boolean field.

that does not mean that every component needs to be handle every kind of scenario. Components should ideally be small and have a single purpose, and if you need additional functionality you should create compositions of different components.

I fully agree with your sentiments, but I don't think this is the applicable place for it. In our case we offer a component that has a feature which in turn comes an inherent problem. We cannot be saying to the consumer:

"Hey, we are aware of this problem, and just wanted to let you know too that this problem exists. You should make sure not to have this problem when you use our component. However, if you wanna prevent having the issue, you should go use another component and fix it in your code."

This would be both counter intuitive and wrong. It is like buying a smartphone which has a certain feature; let say a Turbo Mode. But when the user turns on the Turbo Mode, the phone overheats and turns off. The manufacturer says:
"we know that this problem exist. But we decided not to add a cooling mechanism inbuilt for it, to keep the device more simple. If you want to use the Turbo Mode, go buy a cooler shell for your smartphone. By the way, we also produce and sell cooler shells."

We know the the readonly mode of our boolean components comes with an inherent problem. So the component itself should have means of mitigating the problem. Therefore

that does not mean that every component needs to be handle every kind of scenario.

is not a valid argument here. Switch and Checkbox are already handling the scenario of being readonly by changing their visual design and interactivity. But they are handling it in an insufficient way, forcing the users to have problematic experiences, or forcing the consumer to do loads of extra work to fix it.

We can still add functionality to lime-admin to let this be configurable without consumers creating their own components.
create a component in lime-crm-building-blocks that combines the two

This is not only a Lime Admin problem or a CRM problem. This comes with boolean fields, specially when they're set to readonly. I don't know how you want to fix it or implement this in Lime Admin. I can't do it myself anyways. You guys should find a way for it. But as I tried to explain above, I believe we need to address the issue in the component, not outside of it.

Ok. Lots of discussion here. Sorry if I missed something by not reading all the comments. But I have a suggestion.

Is it enough to handle readonly for checkboxes in limel-form? That is, if the schema type is boolean:

render limel-checkbox if not readonly in schema

render limel-readonly-bool if readonly in schema

Currently in Lime CRM, we're not really specifying a custom component to use for boolean fields anyway.

I guess we don't really need a component that dynamically can toggle its readonly state?

@vicgeralds that would certainly help Lime CRM, but it wouldn't fix the inherent problem of boolean fields, when they become readonly. That'd be we just add documentations to Switch and Checkbox, and ask consumers to not just mindlessly render the boolean field in readonly mode, and instead use limel-readonly-boolean (which means making it public, what Johan is saying) or another proper visualization.

I don't like it too much, but I also don't see this as a show stopper. We can do so too.

Anyhow, I don't know how to fix the issue in the limel-form anyways as you describe. You guys need to do it or help me do it.

Kiarokh · 2024-03-27T08:49:51Z

can we squash?
how do we add this feature to the limel-form, and make it configurable via admin? Is that a next PR or should we give it a try here?

jgroth · 2024-03-27T08:55:44Z

can we squash?

how do we add this feature to the limel-form, and make it configurable via admin? Is that a next PR or should we give it a try here?

Yes squash it 😄
Should it not work already to configure it? 🤔

jgroth · 2024-03-27T11:51:45Z

api-extractor.json

@@ -400,7 +400,7 @@
        "addToApiReportFile": false
      },
      "ae-incompatible-release-tags": {
-        "logLevel": "error",
+        "logLevel": "warning",


@adrianschmidt I think we need to change this to warning. Stencil does not seem to output all the doc blocks, which makes API extractor complain about the release tags not being compatible

Or we might have to set it to none even unless we can get it to exit with a 0 when there are warnings, not sure how to do that yet 🤔

The way to "ignore" it, is to set addToApiReportFile to true. Then it adds the warning to the api report instead of outputting it on the command line, and that also makes the verify-check pass (at least if it's set to "warning").

That's what I've done with ae-missing-release-tag below (line 410-421).

Then we still get the warning in the api report, and can at least review it as part of the PR.

adrianschmidt · 2024-03-27T12:20:06Z

etc/lime-elements.api.md

The reason it looks like every singe line of this file has been changed is likely that someone is using the wrong node version when generating it.

What Node version should be used? I've generated the file on Node 18

.nvmrc says lts/*, which is currently Node 20.

Aside: We should probably specify v20 instead in that file… 🤔

I fixed it and pushed the changes.

The thing is that once the file has been changed (in whatever way it is being changed to make it look like every single line has been changed), you have to remove the file and generate it anew for it to go back to what it was like. I don't know why. Perhaps we should update the api:update script to do that every time… 🤷‍♂️

src/design-guidelines/boolean/examples/radio-buttons.tsx

Kiarokh · 2024-03-28T10:04:34Z

I tried this

{
	"readonlyLabels": [
		{
			"value": true,
			"icon": "news",
			"text": "Is subscribed to receive newsletters"
		},
		{
			"value": false,
			"icon": {
				"name": "cancel_subscription",
				"color": "rgb(var(--color-orange-default))"
			},
			"text": "Is unsubscribed from newsletters"
		}
	]
}

in Lime Admin, and that shows only the default:

Am I doing something wrong?

jgroth · 2024-04-02T09:11:10Z

Am I doing something wrong?

It should work now 🙂

Kiarokh · 2024-04-02T10:18:46Z

{
	"readonlyLabels": [
		{
			"value": true,
			"icon": "news",
			"text": "Is subscribed to receive newsletters"
		},
		{
			"value": false,
			"icon": {
				"name": "cancel_subscription",
				"color": "rgb(var(--color-orange-default))"
			},
			"text": "Is unsubscribed from newsletters"
		}
	]
}

Great! I've also added a final touch, and verified that this works. I'm happy. If you're happy too, please let's merge this so that I can write the guidelines for the platform.

This will enable dynamically changing labels (an optional icon + text) based a give value.

Also - Add guidelines for correctly labeling boolean fields - Use better labels in the default examples of switch and checkbox

A lot of the output from Stencil omit doc block comments, which makes some of the types appear public even if they are marked as beta. This in turn generates the error `ae-incompatible-release-tags`, which would have been nice to have but it seems not to be fully compatible with what Stencil generates.

lime-opensource · 2024-04-02T12:33:05Z

🎉 This PR is included in version 37.13.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Kiarokh added usability Things that make it harder or easier for users to understand or use elements 🦄✨ yolo labels Jan 30, 2024

Kiarokh self-assigned this Jan 30, 2024

Kiarokh force-pushed the readonly-boolean branch from a01788c to 12fd217 Compare January 31, 2024 09:26

Kiarokh force-pushed the readonly-boolean branch 7 times, most recently from a7e869d to e494f40 Compare February 2, 2024 10:06

Kiarokh marked this pull request as ready for review February 2, 2024 10:16

Kiarokh commented Feb 2, 2024

View reviewed changes

src/components/checkbox/checkbox.scss Outdated Show resolved Hide resolved

Kiarokh force-pushed the readonly-boolean branch from 6a23a08 to 4870aef Compare February 2, 2024 13:34

Kiarokh commented Feb 4, 2024

View reviewed changes

jgroth reviewed Feb 5, 2024

View reviewed changes

src/components/checkbox/checkbox.tsx Outdated Show resolved Hide resolved

Kiarokh force-pushed the readonly-boolean branch 2 times, most recently from 02bc3af to 0c5458a Compare February 5, 2024 12:04

Kiarokh requested a review from a team as a code owner February 5, 2024 12:30

This comment was marked as outdated.

Sign in to view

Kiarokh force-pushed the readonly-boolean branch from eeebc25 to 32e988a Compare February 5, 2024 13:51

adrianschmidt mentioned this pull request Feb 5, 2024

False positive for no-restricted-imports lint rule #2766

Closed

This comment was marked as outdated.

Sign in to view

Kiarokh removed their assignment Feb 6, 2024

Kiarokh commented Feb 6, 2024

View reviewed changes

jgroth self-assigned this Mar 18, 2024

jgroth mentioned this pull request Mar 19, 2024

Readonly boolean 2 #2853

Merged

hannahu added the on-hold label Mar 21, 2024

civing removed the on-hold label Mar 25, 2024

jgroth force-pushed the readonly-boolean branch 2 times, most recently from d5c47d2 to 27b16d7 Compare March 27, 2024 11:50

jgroth reviewed Mar 27, 2024

View reviewed changes

adrianschmidt reviewed Mar 27, 2024

View reviewed changes

jgroth force-pushed the readonly-boolean branch from 27b16d7 to d20fdc4 Compare March 27, 2024 13:11

Kiarokh force-pushed the readonly-boolean branch from d20fdc4 to 6a2926b Compare March 27, 2024 13:15

jgroth force-pushed the readonly-boolean branch from 6a2926b to ed1d34c Compare March 27, 2024 13:22

adrianschmidt force-pushed the readonly-boolean branch from ed1d34c to 6106afa Compare March 27, 2024 13:45

jgroth reviewed Mar 27, 2024

View reviewed changes

src/design-guidelines/boolean/examples/radio-buttons.tsx Outdated Show resolved Hide resolved

Kiarokh force-pushed the readonly-boolean branch 3 times, most recently from 803a038 to 856c6c2 Compare March 27, 2024 15:18

Kiarokh force-pushed the readonly-boolean branch from 075c699 to 40dc62e Compare April 2, 2024 09:49

Kiarokh and others added 6 commits April 2, 2024 14:10

feat(dynamic-label): add new component

9499fa1

This will enable dynamically changing labels (an optional icon + text) based a give value.

feat(switch): allow customizing the visualization of readonly state

6fc7e3a

feat(checkbox): allow customizing the visualization of readonly state

ee38fd2

docs(switch & checkbox): add guidelines about readonly state

310b79b

Also - Add guidelines for correctly labeling boolean fields - Use better labels in the default examples of switch and checkbox

fix(form): allow additional properties for boolean fields

e7841ff

jgroth force-pushed the readonly-boolean branch from a36c86a to e7841ff Compare April 2, 2024 12:10

jgroth approved these changes Apr 2, 2024

View reviewed changes

Kiarokh merged commit 6113e6e into main Apr 2, 2024
11 checks passed

Kiarokh deleted the readonly-boolean branch April 2, 2024 12:30

lime-opensource added the released label Apr 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Readonly boolean #2747

Readonly boolean #2747

Kiarokh commented Jan 30, 2024 •

edited

Loading

github-actions bot commented Jan 31, 2024

Kiarokh Feb 4, 2024 •

edited

Loading

jgroth Feb 5, 2024

vicgeralds Feb 5, 2024

Kiarokh Feb 5, 2024

vicgeralds Feb 5, 2024

jgroth Feb 5, 2024

adrianschmidt Feb 5, 2024

civing Feb 5, 2024

This comment was marked as outdated.

This comment was marked as outdated.

Kiarokh Feb 6, 2024

jgroth Feb 7, 2024

adrianschmidt Feb 7, 2024

Kiarokh Feb 8, 2024 •

edited

Loading

vicgeralds Feb 8, 2024 •

edited

Loading

Kiarokh Feb 8, 2024

Kiarokh commented Mar 27, 2024

jgroth commented Mar 27, 2024

jgroth Mar 27, 2024

jgroth Mar 27, 2024

adrianschmidt Mar 27, 2024

adrianschmidt Mar 27, 2024 •

edited

Loading

jgroth Mar 27, 2024

adrianschmidt Mar 27, 2024

adrianschmidt Mar 27, 2024

Kiarokh commented Mar 28, 2024

jgroth commented Apr 2, 2024

Kiarokh commented Apr 2, 2024

lime-opensource commented Apr 2, 2024

Readonly boolean #2747

Readonly boolean #2747

Conversation

Kiarokh commented Jan 30, 2024 • edited Loading

Review:

Browsers tested:

github-actions bot commented Jan 31, 2024

Kiarokh Feb 4, 2024 • edited Loading

Choose a reason for hiding this comment

Other relevant and similar issues:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as outdated.

This comment was marked as outdated.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kiarokh Feb 8, 2024 • edited Loading

Choose a reason for hiding this comment

vicgeralds Feb 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kiarokh commented Mar 27, 2024

jgroth commented Mar 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrianschmidt Mar 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kiarokh commented Mar 28, 2024

jgroth commented Apr 2, 2024

Kiarokh commented Apr 2, 2024

lime-opensource commented Apr 2, 2024

Kiarokh commented Jan 30, 2024 •

edited

Loading

Kiarokh Feb 4, 2024 •

edited

Loading

Kiarokh Feb 8, 2024 •

edited

Loading

vicgeralds Feb 8, 2024 •

edited

Loading

adrianschmidt Mar 27, 2024 •

edited

Loading