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

Feature request: more powerful sections #31

Open
Caedendi opened this issue Nov 29, 2024 · 8 comments
Open

Feature request: more powerful sections #31

Caedendi opened this issue Nov 29, 2024 · 8 comments

Comments

@Caedendi
Copy link

Caedendi commented Nov 29, 2024

Hi Oleg,

You previously implemented my request to add a way to enable/disable all settings in a section (thanks again for that). I have since spent a lot of time tinkering with sections and various kinds of settings in the past year while developing v4 of my extended filter and have found that the current implementation is imperfect and not ideal. I want to suggest a simpler and more powerful solution.

My proposal: let sections simultaneously act as a value

Instead of the section reading its contents and automatically doing something special itself when it only contains checkboxes, I suggest removing that functionality and replacing it with functionality where a section simply can optionally also be a value itself.

Checkboxes: the obvious one

The most obvious one of course would be the checkbox. Just it being a simple checkbox would give modders total freedom in code behavior and in conjunction with the existing setting visibility toggling provides an easy and intuitive solution for both modders and users.

But don't limit it to only checkboxes

Regarding other section value types, I can think of a situation in my extended filter where I would like a section to also act like a dropdown list instead (it's for the Loot Filter => Stats & Modifiers => Show Equipment Quality section). Just don't limit the value types a section can contain (instead of course, another section), let modders have total freedom in creativity and be surprised with what they come up with.

Issue with the current implementation

Currently, if a section contains only checkboxes, you have provided an interactable checkbox inside the section element to toggle all of 'em at once. If these checkboxes are all enabled by default however, the section toggle itself still shows as disabled by default and clicking it will enable the toggle and disable all containing checkboxes (correctly). It functions like it should from a logical/programming viewpoint but it looks weird. And then there's the mixed scenario where one checkbox is enabled by default and one is disabled... I wouldn't want to think of a solution for that from a user viewpoint. This will be a non-issue with the above proposal.

Please expand visibility toggling to sections too

On a last note, I'd love it if the visibility feature could also be expanded to sections. If a section should not be visible, please hide the element and all its contents. Don't do anything else; that's the modder's responsibility. Maybe description could be added too, either inside the element or below it when expanded.

Would love to hear what you think about all of this.

@olegbl
Copy link
Owner

olegbl commented Nov 29, 2024

If these checkboxes are all enabled by default however, the section toggle itself still shows as disabled by default

It shows what the state will be upon clicking the button:
https://github.com/olegbl/d2rmm/blob/master/src/renderer/react/settings/ModSettingsSection.tsx#L147

Maybe bad UX, but it does calculate things correctly. I can invert it.

EDIT: Oh, I know why I did this. If it was inverted, there wouldn't be a good way to represent what the current state is when some things are checked. If it shows what the state will be after clicking the button, there's always a valid icon to use.

I'd love it if the visibility feature could also be expanded to sections

Seems reasonable.

EDIT: 9038e9a

replacing it with functionality where a section simply can optionally also be a value itself

I think the UI will be kind of janky if the section header is also an interactive element. e.g. Text input where if you click slightly outside of it, it ends up collapsing the whole section is a UI that's prone for frustration.

I would recommend you put an interactive element of your choice as the first child of that section instead.

@Caedendi
Copy link
Author

Caedendi commented Dec 1, 2024

It shows what the state will be upon clicking the button

Ah, that explains, but yeah bad UX imo. I'd rather have that be inverted and be on enabled mode when at least one of the toggles is set to enabled. It would act a bit buggy if the section's master toggle is switched off but one of the hidden underlying toggles is still on, but I'd argue that's still a more intuitive design (better UX) than the current one.

I would recommend you put an interactive element of your choice as the first child of that section instead.

That's why I'm doing right now but to me that's what's feeling janky.

I didn't think of text/number input fields (it was pretty late okay don't judge) and can't imagine those ever being a good UI/UX design, but I still stand behind the idea of allowing a section to be able to have an embedded checkbox/dropdown in its header. It could work (not break anything) if the current "toggle all" will be the default functionality but have an opt-out, and that an embedded checkbox/select will override the toggle-all if enabled.

I think the UI will be kind of janky if the section header is also an interactive element. e.g. Text input where if you click slightly outside of it, it ends up collapsing the whole section is a UI that's prone for frustration.

But that's already the case with the "all checkboxes" toggle and the "revert to default" toggle. You may still disagree with the feature request but that's a bad argument;)

Maybe description could be added too, either inside the element or below it when expanded.

And what about descriptions for headers? They're already optional for the other config fields, I doubt that would break anything.

Just noticed something else btw. Right now, when clicking the toggle all button and the revert button shows up, the toggle all button will jump to the left. Would you agree that it could better appear left of the toggle all button instead of right?

Imo this order for elements in section headers would give the best UX:
[title] [revert] [toggle all or embedded toggle/dropdown] [description] [expand]

@olegbl
Copy link
Owner

olegbl commented Dec 1, 2024

I'd rather have that be inverted and be on enabled mode when at least one of the toggles is set to enabled.

I suppose I can still invert it if I switch to MUI's checkbox icons since they have an intermediate variant (unlike the toggle icons). It'd be a bit inconsistent with the checkbox field UI, but probably not a big deal.

That's why I'm doing right now but to me that's what's feeling janky.

Can you elaborate why it feels janky? Some screenshots to explain the problem would help.

But that's already the case with the "all checkboxes" toggle and the "revert to default" toggle.

Yeah, I don't really love the current state either. I just think it would matter more with this feature as people would interact with the headers more.

And what about descriptions for headers?

Yeah, that seems fine.

Would you agree that it could better appear left of the toggle all button instead of right?

I don't think it matter much since it will never jump underneath the cursor, but I don't have any real problems with switching them either.

olegbl added a commit that referenced this issue Dec 1, 2024
…mediate state, inverted the logic, and improved the tooltip. (Partially addresses #31)
@Caedendi
Copy link
Author

Caedendi commented Dec 1, 2024

Toggle All button behavior

Would you agree that it could better appear left of the toggle all button instead of right?

I don't think it matter much since it will never jump underneath the cursor, but I don't have any real problems with switching them either.

But it does?

101 toggle all

Screenshots for header toggles

Can you elaborate why it feels janky? Some screenshots to explain the problem would help.

Of course. Here goes.

All off

Left is how it is now: pretty cluttered. Notice how there's two toggles in the Show Item Level section while it's completely disabled and they each say something different.

Right is how I'd want it to be. All settings in a section would disappear if it's toggled off. In this example Show Equipment Quality does not have a revert button since that's off by default, where the other 3 would be on by default. With the defaultExpanded json setting set to true, the section would also auto-expand as soon as you enable it. Pretty nifty.

102 subsections all off

Multiple on

Again, Left is how it is now. The enable buttons take up quite some space and attention span, making the Stats & Modifiers section pretty cluttered.

Right is how I'd want it to be. Moving the enable button to the section header will show that there's only 5 actual settings here that should currently take up your attention span, making it a lot easier on the eyes.

The toggle all button could still be always present if more than 1 checkbox lives in a subsection, just move it to an intermediate spot between the section header and the first element.

On another note, a tiny little space between folded subsections wouldn't hurt either.

103 subsections multiple enabled

Header dropdown

Lemme also show you too how a dropdown in a section header can be of increased UX value. This section basically has 4 states: disabled, prefix, suffix and both.

  • Disabled:
    • entire section folded
  • Prefix or Suffix:
    • The item name will have a tag that looks something like this: Phase Blade [E]
    • On this setting you'll have the option to set both the character style and the brackets style.
  • Both
    • Now the item name will look something like this: ·Dimensional Blade· or :Phase Blade:
    • The "Indicator Style" setting here is actually an entirely different element, but sharing the same name
    • This setting of course does not have brackets (only character styling) so that setting disappears

104 equipment quality

Custom quality tags

Setting the style to custom will allow the user to create the tag in its entirety, so the bracket style setting will be replaced with 3 text input fields. For the Prefix & Suffix setting this would even summon 6 input fields, so with the features requested in this thread I could neatly store those in another subsection that only appears when necessary.

9 4 dropdown prefix custom

@olegbl
Copy link
Owner

olegbl commented Dec 1, 2024

Thanks for the detailed description of what are trying to achieve!

I think the only thing that's still in question is the ability to have a custom input element in the section header. (The rest have commits linked in this issue.)

It doesn't actually sound like you'd want the real fields in the section header (which is what I was assuming), but rather just want to have some control over the logic of the toggle button in the section header (where it would be a separate value and enables/disables the entire section.)

I'd probably not want to mix the UX with the toggle-all feature, but an optional "value" property on sections that causes a toggle to appear to the left of the name (and the section cannot be expanded while the toggle is off) seems like it could work. I might also want to add a property to disable the toggle all button (though it can already be achieved via a hidden non-checkbox field.)

@Caedendi
Copy link
Author

Caedendi commented Dec 2, 2024

Thanks for the detailed description of what are trying to achieve!

You're very welcome.

I think the only thing that's still in question is the ability to have a custom input element in the section header. (The rest have commits linked in this issue.)

Oh wow I totally scrolled past those commits:'D Didn't expect that it would happen so fast. Changing bindings like and to accept an endless array instead of just 2 values (e.g. this commit) is also a very welcome change. I have some chained ands that I can clean up now.

It doesn't actually sound like you'd want the real fields in the section header (which is what I was assuming), but rather just want to have some control over the logic of the toggle button in the section header (where it would be a separate value and enables/disables the entire section.)

No that was indeed what I originally requested, but I have since mostly come back from that; instead of allowing all mod config types in the section header to mostly just:

  • checkbox
    • what you're describing is right, though I'd phrase the last part as "and allows you to enable/disable the entire section". Leave the handling of the code logic and visibility to the modder. I must admit though that atm I can not think of a situation where you'd want some of your settings to show in a section while you have that section disabled so you may be right in that D2RMM could just automatically hide everything inside. Food for thought.
  • preferably select (dropdown) too
    • like in the pic below, enabling more states than the 2 that checkbox allows (I could've made that a bit clearer in my previous post)
    • (to be clear: Enable will of course just be another list entry, not some weird element-in-element nonsense)
    • here you'd definitely just want to leave visibility up to the modder, saves you some headaches

105 dropdown header

I'd probably not want to mix the UX with the toggle-all feature, but an optional "value" property on sections that causes a toggle to appear to the left of the name (and the section cannot be expanded while the toggle is off) seems like it could work. I might also want to add a property to disable the toggle all button

Sounds great!

(though it can already be achieved via a hidden non-checkbox field.)

I didn't know that, how does that work?

PS: everything else has indeed been addressed with the linked commits, though I think maybe you skipped over this one:

On another note, a tiny little space between folded subsections wouldn't hurt either.

@olegbl
Copy link
Owner

olegbl commented Dec 2, 2024

I didn't know that, how does that work?

Just add a placeholder field that you don't use, and give it visibility of false, and a type of anything other than a checkbox. The "toggle all" button only shows if all children of a section are checkboxes (though if I add a config option for it, perhaps that's one aspect that can be customized.)

@Caedendi
Copy link
Author

Caedendi commented Dec 2, 2024

Just add a placeholder field that you don't use, and give it visibility of false, and a type of anything other than a checkbox. The "toggle all" button only shows if all children of a section are checkboxes (though if I add a config option for it, perhaps that's one aspect that can be customized.)

Oh lol that is literally what you said, a quick and easy hack. I should really stop coding so late. But yeah an optional property for that would be nice.

So after a good night's sleep I wanna propose starting with only the checkbox functionality in the section header since 95% of my current issues will be fixed with that. For that Equipment Quality section an argument can be made that having an on/off toggle in the header and a separate field for Position would have equal or arguably better UX than a dropdown. So for now let's put that in the freezer and if I run into another example where the dropdown would be a better UX than a checkbox then I'll get back to you.

Thanks for thinking along and implementing it all so quickly!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants