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

Rule & Script editor: Various improvements #2362

Merged
merged 4 commits into from Feb 18, 2024

Conversation

jimtng
Copy link
Contributor

@jimtng jimtng commented Feb 16, 2024

Various improvements to the rule editor and script editor

  • Remove/hide non-functional title/description input fields in the initial Add Trigger/Action/Condition for the rule editor. These inputs don't do anything. They became functional once a type of module is selected, and they will indeed show up then.
  • When editing a rule module, set the popup title to reflect the type of module, i.e. "Edit Trigger", or "Edit Condition"
  • Changed the link on the top right corner from Done to Save
  • Removed the pencil icon for consistency. Previously, Cron and Scripts have a pencil icon which goes to the Edit module popup vs clicking directly on the line goes directly to Cron Builder, or Script Editor. Now with the pencil icon gone, cron will go into the Edit module popup, which can lead to the cron builder from there, and Scripts go directly to script editor. Changing the script title/description is possible from within the script editor's bottom area as shown below.

Script Editor:

  • Change the title of Script Editor, from "Edit Script" to better clarify what type of script is being edited. So the new title becomes "Edit Action Script: <script title>" or "Edit Condition Script: <script title>"
  • Add the ability to change the script's title and description after they're created.

Remove the Title/Description input in the Add [Trigger/Action/Condition]

They don't seem to apply to anything

Before:
image

After:
image

Change the popup title from Edit module to Edit [Trigger/Action/Condition]

Before:
image

After:
image

Change the title of script editor

Before:
image

After:
image

And when the action has a custom label:
image

So user can see whether it's an action script or a condition script from the editor's title
image

Removed pencil icons

Before:
image

After:
image

Allow changing the script module's title and description

Before:
Currently it's not possible to do this. There is no input / way to do it
image

After:
image

Also the "Scripting Language" selection has been changed into a smart-select sheet selection.
image

Copy link

relativeci bot commented Feb 16, 2024

Job #1584: Bundle Size — 11.01MiB (~+0.01%).

57f87e1(current) vs af6174b main#1582(baseline)

Warning

Bundle contains 19 duplicate packages – View duplicate packages

Bundle metrics  Change 2 changes Regression 1 regression
                 Current
Job #1584
     Baseline
Job #1582
No change  Initial JS 1.84MiB 1.84MiB
Regression  Initial CSS 607.89KiB(+0.03%) 607.73KiB
Change  Cache Invalidation 23.24% 18.14%
No change  Chunks 220 220
No change  Assets 242 242
No change  Modules 3087 3087
No change  Duplicate Modules 164 164
No change  Duplicate Code 1.77% 1.77%
No change  Packages 150 150
No change  Duplicate Packages 18 18
Bundle size by type  Change 2 changes Regression 1 regression Improvement 1 improvement
                 Current
Job #1584
     Baseline
Job #1582
Regression  JS 9.2MiB (~+0.01%) 9.2MiB
Improvement  CSS 889.37KiB (~-0.01%) 889.37KiB
Not changed  Fonts 526.1KiB 526.1KiB
Not changed  Media 295.6KiB 295.6KiB
Not changed  IMG 140.74KiB 140.74KiB
Not changed  HTML 1.24KiB 1.24KiB
Not changed  Other 871B 871B

View job #1584 reportView jimtng:rule-module-popup-title branch activityView project dashboard

@jimtng jimtng marked this pull request as ready for review February 16, 2024 15:57
@jimtng jimtng requested a review from a team as a code owner February 16, 2024 15:57
@rkoshak
Copy link

rkoshak commented Feb 16, 2024

Remove the Title/Description input in the Add

Is there no way to make it work from there?

Maybe there's a different way it can be adjusted so it's not so awkward to change them label (e.g. make the label and description editable from the main rule's page instead of needing to click on the > or pencil icon to change it? I suspect a good number of users don't even know these can be changed (even though I did call it out in a number of places including Getting Started).

Your changes for Scripts are a great way to address Scripts though and I really like it. But I miss something similar for triggers, other actions and other conditions. Though in those cases the default generated label and description is pretty good.

All the rest of the changes are 👍 in my book.

@jimtng
Copy link
Contributor Author

jimtng commented Feb 16, 2024

@rkoshak (and others) I'd appreciate your feedback for this PR, thanks!

Is there no way to make it work from there?

To me, it doesn't seem to make a lot of sense letting them edit it there because they haven't even defined what module to use. Furthermore, the placeholders can't provide the default values because they depend on the selected module type, which the user hasn't selected yet.

pencil icon to change it? I suspect a good number of users don't even know these can be changed

Ha! I did not even know that the pencil icon leads you to something different with a small popup script editor! I was focusing only on the big editor (not clicking on the pencil icon). Had you not mentioned it here, I wouldn't have known.

Letting them edit directly on the rules page, hmm... I can give that a try, but in a separate PR. I am trying to keep this small.

Your changes for Scripts are a great way to address Scripts though and I really like it. But I miss something similar for triggers, other actions and other conditions.

Hmm I wonder if you realised that you can actually change them. Click on the existing trigger / other actions, and those two input fields are there at the top, you can type in them!

image

At some point I was going to add labels for those, but decided not to. Here's what it would look like with labels:

image

@jimtng
Copy link
Contributor Author

jimtng commented Feb 16, 2024

Or with an "info" subtitle to make it even more obvious (hopefully)

image

@rkoshak
Copy link

rkoshak commented Feb 16, 2024

because they haven't even defined what module to use

True, but the user knows what they want this trigger/condition/action to do at that point. The label and description should cover the "what" more than the "how" (they can click on it and look at the config to see the how) and the "what" is independent from what ever module is used.

If they choose to fill in their own label and description, the defaults from the module wouldn't apply. Only if they are left blank would the defaults from the module get added. And does that actually get added? It doesn't show up in the Code tab unless the user overrides it with their own. I always assumed the default label and description was generated dynamically in the browser.

Letting them edit directly on the rules page, hmm... I can give that a try, but in a separate PR. I am trying to keep this small.

👍

Hmm I wonder if you realised that you can actually change them. Click on the existing trigger / other actions, and those two input fields are there at the top, you can type in them!

Oh yes, I know you can change them from there. But it's awkward to:

  1. create the trigger
  2. click on it again to set the label and description (and having to know that I can change them by clicking on them)
  3. Oh, but not if it's an Action. There you can't just click on them, you have to use the pencil icon.

It's not super intuitive and definitely awkward which is one reason I don't thing a lot of people use these to help documented their rules better.

I personally am not as concerned about making it obvious that you can edit the label and description in that secondary dialog as I am concerned about the fact that it works differently between triggers and conditions from actions and you have to click through to change it after the fact instead of being able to set it while you are creating the trigger/condition/action in the first place. That extra step is annoying.

@jimtng
Copy link
Contributor Author

jimtng commented Feb 17, 2024

Oh yes, I know you can change them from there. But it's awkward to:

  1. create the trigger
  2. click on it again to set the label and description (and having to know that I can change them by clicking on them)
  3. Oh, but not if it's an Action. There you can't just click on them, you have to use the pencil icon.

The pencil icons are shown for:

  1. Cron triggers
  2. Script (Actions / Conditions)

After pondering a bit about this, I think that there's an inconsistency in the current UI that caused this confusion.

Take these two for example:
image

The first one doesn't have the pencil icon, the second one does.

When no pencil icon is visible for the module, clicking anywhere on the line brings you to the module editor (rule-module-popup.vue), such as this:
image

However, for those two above (Cron and Script), the pencil icon is visible, then:

  • clicking on the pencil icon brings you to the module editor
  • clicking on the line outside the pencil icon, brings you directly to script editor or cron builder.

Therein lies the inconsistency. This behaviour should be flipped:

  • Clicking on the line outside the pencil icon, should bring you to the module editor, just like those without a pencil icon, so you can equally edit the title/description in the same manner
  • Clicking on the special pencil icon brings you to the special route, i.e. directly editing the script or cron builder.

WDYT, @rkoshak, @ghys, @florian-h05 ?
Changing this however, will no doubt cause a surprise because we're already used to the current behaviour.

I understand that the current behaviour makes for a more efficient / streamlined process, because once the module was chosen, people mostly just want to edit the cron/script.

BTW this "behaviour flip" isn't currently a part of this PR, and indeed is not what this PR is about. Depending on our consensus, it can be implemented in a different PR

@rkoshak
Copy link

rkoshak commented Feb 17, 2024

I don't have a problem with flipping it but wonder if this is one of those rare cases where we can have our cake and eat it too.

What if instead of flipping the behavior we added the pencil icon and removed the "click anywhere/>" from those triggers/actions/conditions to don't have a separate editor? Then the most common action remains streamlined but the way to get to the regular config dialog is more consistent.

@jimtng
Copy link
Contributor Author

jimtng commented Feb 17, 2024

What if instead of flipping the behavior we added the pencil icon and removed the "click anywhere/>" from those triggers/actions/conditions to don't have a separate editor? Then the most common action remains streamlined but the way to get to the regular config dialog is more consistent.

good idea, except, I like how curently I can click anywhere, within a large area rather than having to do a precision click on the pencil icon. This ability to click anywhere speeds up the process, which is probably why it was designed like this in the first place

@rkoshak
Copy link

rkoshak commented Feb 17, 2024

It's maybe a cludge but what it we simply add the pencil icon but leave the click anywhere? Can we remove the ">" symbol too?

Then maybe it will be more clear what to expect when clicking.

I do realize this mostly just moves the confusion and doesn't really solve it. But maybe it will help some users if the pencil icon is consistently shown.

Another idea would be to eliminate the pencil icon entirely. Embed the "metadata" fields into the script editor and cron builder dialogs so there is no need to have two separate dialogs for those (and any future trigger/action/condition that needs it's own dialog). I don't know if that's feasible but it certainly would normalize the rule's page. They would all work the same.

@jimtng
Copy link
Contributor Author

jimtng commented Feb 17, 2024

Another idea would be to eliminate the pencil icon entirely. Embed the "metadata" fields into the script editor and cron builder dialogs so there is no need to have two separate dialogs for those (and any future trigger/action/condition that needs it's own dialog). I don't know if that's feasible but it certainly would normalize the rule's page. They would all work the same.

This can be done:

  • With Cron it will show this:
image and when you clicked "Build", it goes to the cron builder. Right now, clicking on the bigger area brings you directly to the cron builder, and clicking on the pencil icon brings you to the screenshot above (module editor)

So what you're saying above is, when clicking on the bigger area (no more pencil icon) it should go to the module editor. And one more click to get to the cron builder.

  • With scripts, and with this PR, you can edit the module title + description in the script editor. You won't be able to get to "Module editor" for the script.

How does that sound? The only downside is one extra click to edit cron.

@jimtng
Copy link
Contributor Author

jimtng commented Feb 17, 2024

The only downside is one extra click to edit cron.

Actually, I think this is not a downside for people who want to edit the raw cron expression, it's actually a plus.

@florian-h05
Copy link
Contributor

This behaviour should be flipped:

Agreed 👍

How does that sound?

Sounds fine to me.

The only downside is one extra click to edit cron.

Yes, might be true, but I don't think it is a large downside, especially compared to the improved consistency.

This really reduces overall complexitiy of the rule editor and makes it more consistent, which I think is always a good thing.

So it seems like this PR is not ready for review yet, correct?

@ghys
Copy link
Member

ghys commented Feb 17, 2024

Since I designed those screens, a few remarks:

Remove the Title/Description input in the Add [Trigger/Action/Condition]

I'm good with it, especially in that 4-button screen.

Change the popup title from Edit module to Edit [Trigger/Action/Condition]

Nice, but you changed the upper-right "Done" from "Save" - it is really a save though?

Change the title of script editor
Allow changing the script module's title and description

👍

Also the "Scripting Language" selection has been changed into a drop down selection.

I want to avoid native dropdowns in general as their rendering can be quite ugly sometimes. I don't think this is necessary.
Note that the "module editor" was originally a generic UI to edit the rule module (different than the "add module" UI), and so just present the module title/description and its parameters. Your proposal is however interesting - we could have a smart select to change the language - but I still prefer the radio button list.

@ghys
Copy link
Member

ghys commented Feb 17, 2024

At some point I was going to add labels for those, but decided not to. Here's what it would look like with labels:
image

I had the same process but also decided not to, but in the end, it's not looking too bad!

@jimtng
Copy link
Contributor Author

jimtng commented Feb 18, 2024

Nice, but you changed the upper-right "Done" from "Save" - it is really a save though?

Indeed it's a Save, because if you clicked the Back link (top left) instead, the edited values will be discarded/not saved.
The only thing I haven't paid attention to is "dirty" warnings.

I want to avoid native dropdowns in general as their rendering can be quite ugly sometimes. I don't think this is necessary.

I tried reverting to the radio buttons but it takes up too much vertical space, even when only 2 languages installed (rulesdsl + ecma or ruby), causing a vertical scroll bar to show up. So I've converted it to smart-select with a popover sheet instead.

Screenshot below

I had the same process but also decided not to, but in the end, it's not looking too bad!

Should I add the titles/labels for the input fields on the create/edit module popup?

@jimtng
Copy link
Contributor Author

jimtng commented Feb 18, 2024

So I've converted it to smart-select with a popover instead.

oh no, popovers are too narrow, the ECMAScript got truncated! Changed to sheet instead. New look:
image

@jimtng
Copy link
Contributor Author

jimtng commented Feb 18, 2024

Updated the original post with the latest state + screenshots

@jimtng
Copy link
Contributor Author

jimtng commented Feb 18, 2024

Note: Now that Scripts don't go to rule-module-popup, perhaps these can be removed:

  • parameter-script.vue
  • ParameterScript + related code removed from config-parameter.vue

Signed-off-by: Jimmy Tanagra <jcode@tanagra.id.au>
Signed-off-by: Jimmy Tanagra <jcode@tanagra.id.au>
Signed-off-by: Jimmy Tanagra <jcode@tanagra.id.au>
Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
Copy link
Contributor

@florian-h05 florian-h05 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

I will take care of the dirty handling in a seperate PR.

@@ -296,3 +296,16 @@ html

.sitemap-validation-dialog
--f7-dialog-width 80%

// Align smart-select item with f7-list-input fields
.aligned-smart-select
Copy link
Contributor

Choose a reason for hiding this comment

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

Very nice to have this style now available globally!

emits: ['newLanguage'],
components: {
RuleGeneralSettings
},
computed: {
editable () {
console.log('module')
Copy link
Contributor

Choose a reason for hiding this comment

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

I will remove this logging, seems like a leftover from development.
FYI to inspect the value of properties during development, the Vue DevTools browser extension is a great tool: https://github.com/openhab/openhab-webui/blob/main/bundles/org.openhab.ui/CONTRIBUTING.md#vue-devtools.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

LOL thanks! I installed this but haven't taken the time to learn how to use it.

@florian-h05 florian-h05 changed the title rule-editor, script-editor: various improvements Rule & Script editor: Various improvements Feb 18, 2024
@florian-h05 florian-h05 added the enhancement New feature or request label Feb 18, 2024
@florian-h05 florian-h05 added the main ui Main UI label Feb 18, 2024
@florian-h05 florian-h05 added this to the 4.2 milestone Feb 18, 2024
@florian-h05 florian-h05 merged commit 3f02477 into openhab:main Feb 18, 2024
6 checks passed
@jimtng jimtng deleted the rule-module-popup-title branch February 18, 2024 13:17
jimtng added a commit to jimtng/openhab-webui that referenced this pull request Feb 22, 2024
Missed these in openhab#2362

Signed-off-by: Jimmy Tanagra <jcode@tanagra.id.au>
florian-h05 pushed a commit that referenced this pull request Feb 22, 2024
Missed these in #2362.

Signed-off-by: Jimmy Tanagra <jcode@tanagra.id.au>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request main ui Main UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants