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

Implement #1449 - speed up configuration workflow #1450

Merged
merged 6 commits into from
Jun 29, 2021
Merged

Implement #1449 - speed up configuration workflow #1450

merged 6 commits into from
Jun 29, 2021

Conversation

Abrynos
Copy link
Member

@Abrynos Abrynos commented Jun 24, 2021

Description

Implements #1449 for InputFlag, InputSet and InputList.

Screenshots

image

Additional remarks

I'd propose renaming the three above mentioned components to contain the n word "Enum", since they are exclusively used for that and the name implies a more generic usage (e.g. them also being used for config fields such as GamesPlayedWhileIdle, which is also ImmutableHashSet<...> under the hood), but I have no idea of how to do this.

Checklist

  • My pull request is not a duplicate
  • I added a descriptive title to this pull request
  • I added a concise description or a self-explanatory screenshot to this pull request
  • My code follows the code style of this project
  • I have performed a self-review of my own code

@Abrynos
Copy link
Member Author

Abrynos commented Jun 24, 2021

@MrBurrBurr How do I add a string to localization? Currently the "Select Option" is hardcoded with a TODO.

@Botan626
Copy link
Contributor

Botan626 commented Jun 24, 2021

  1. I don't like Select Option sign for 3 reasons:
    a. It's disturbing, as it can be interpreted as an actual config property value,
    b. We don't select options, we select property values,
    c. Creates too much text on screen.

  2. Dropdown menu needs to be opened every time in order to add another property value, dropdown menu with checkboxes will be a lot more convenient.

@Abrynos
Copy link
Member Author

Abrynos commented Jun 24, 2021

@Botan626 Thank you for your feedback. I would like to adress your concerns:


it can be interpreted as an actual config property value

It can not, since it will never be shown in the dropdown menu it is not selectable. In the place it is in no other text will ever be shown as well so it can't be mistaken after choosing another option either.


We don't select options, we select property values

I agree. With "option" I meant the actual values, since the user has several options to choose from. This goes in hand with the cambridge dictionary entry: "one thing that can be chosen from a set of possibilities". I admit that wording possibly could be improved and you can suggest a change at any time.

Feel free to suggest a good [...] solution. Just saying "it's ugly" won't fix anything

~ @MrBurrBurr, 2021-06-05, 15:50, GMT+2, here


Creates too much text on screen

Previously we displayed one of the options in the same place in addition to the "Add"-button that I removed. Since most of those values have quite long names I would assume the average amount of characters on screen to be decreased 😉 Apart from that: I tried it without any text there and it looks just bad and empty in my opinion. You're welcome to check it out yourself and if more people agree I will change it accordingly.


dropdown menu with checkboxes will be a lot more convenient

Then send your own PR. I chose this solution for several reasons.

  • First and foremost I am able to re-use all the design-decisions made previously by the team that's actively developing and maintaining ASF-ui (big shoutout to @MrBurrBurr and @Aareksio ♥ )
  • I only have to change a few tiny pieces of logic instead of creating a whole new component (e.g. DropdownSelector) with a lot of functionality and using it all over the place (for list, set and flag inputs). I know my limitations. I am not a frontend-dev nor particularly good at GUI design and I imagine such a PR staying open for months before it reaches a state at which the maintainers of this repository would consider it mature enough to merge.
  • I don't have to think about how to get the above component working with InputList where the order of selection matters (e.g. configuration property FarmingOrders), without having two (or rather three) extremely similar types of input being displayed in two completely different ways.

@Botan626
Copy link
Contributor

It can not, since it will never be shown in the dropdown menu it is not selectable. In the place it is in no other text will ever be shown as well so it can't be mistaken after choosing another option either.

It exactly can, because that field already shows selected values.

image

Currently next to the Add button ui shows one of the values, which was not actually selected and is not part of config. Which is actually confusing too. Several times I chose a value in the past, didn't press Add button and saved config.

you can suggest a change at any time

I already said 'we select property values'.

I tried it without any text there

This I believe is the best choice, no unnecessary text on screen, which is disturbing and which you have to read forcedly, because it's just there in front of you. I would like to see a version with empty fields.

I chose this solution for several reasons.

Yes, you gave me your reasons, but all together they say you can't and don't want to make dropdown menu with checkboxes. But do you think it will be a better choice?

@Abrynos
Copy link
Member Author

Abrynos commented Jun 24, 2021

It exactly can, because that field already shows selected values.

please download the build output and try it out yourself.

I already said 'we select property values'.

I think "Select property value" is even worse than "Select option". I'd like to keep the text as short as possible to make it look good on mobile devices as well.

I would like to see a version with empty fields.

I'm not pushing a commit just so you have a build output. You can get that by forking my repository/branch and just removing the text from the source files (you can do this in your browser as well. GitHub supports it). You'll get build artifacts from the github actions tab on your forked version 😉

do you think it will be a better choice?

not really. If we did that and I wanted to see at a glance, which values are currently selected, I'd have to open the dropdown specifically for that. Currently we can see what is selected without opening the dropdown.

@Botan626
Copy link
Contributor

please download the build output and try it out yourself.

Did you see the screenshot? What do you see on it in that fields?
And you think I'm giving my feedback without installing and trying it? Really? Yes or no, Abry?

I think "Select property value" is even worse than "Select option". I'd like to keep the text as short as possible to make it look good on mobile devices as well.

Just 'Property values' will be better imo, as it's obvious user is going to select them or not select them.

If we did that and I wanted to see at a glance, which values are currently selected, I'd have to open the dropdown specifically for that. Currently we can see what is selected without opening the dropdown.

No, you would still see selected values like here:
image

If you open dropdown menu, selected values will be checked. You want to remove any value? Uncheck it, close dropdown menu and go edit another property. Or just press Save button right away after you uncheck it without doing extra click to close it. Very handy and convenient imo. What do you think about dropdown menus with checkboxes now?

You can get that by forking my repository/branch and just removing the text from the source files (you can do this in your browser as well. GitHub supports it). You'll get build artifacts from the github actions tab on your forked version 😉

I just remembered I still have scripts to build ASF-ui.

@Botan626
Copy link
Contributor

Here is a comparison.

  1. Current verson.
  2. Your PR version,
  3. Your PR without 'Select option' sign version.
Screenshots

chrome_lUIyB6aiW8

chrome_BEuFQnAnmU

chrome_ShhSZE8GBZ

I would choose without hesitation 3d version, because I can see my selected properties' values with quick glance. And if the fields had the same length, as they do now with Add button, it'd have looked even better.

@JustArchi JustArchi marked this pull request as draft June 26, 2021 11:09
@MrBurrBurr
Copy link
Member

How do I add a string to localization?

You can add your string here: https://github.com/JustArchiNET/ASF-ui/blob/main/src/i18n/locale/default.json
And then use it like this: this.$t('your-string');

For more information (about for example pluralization or placeholder) you can check out this neat documentation that Mole wrote: https://github.com/JustArchiNET/ASF-ui/tree/main/src/i18n/lib#usage

@Abrynos
Copy link
Member Author

Abrynos commented Jun 26, 2021

How do I add a string to localization?

You can add your string here: https://github.com/JustArchiNET/ASF-ui/blob/main/src/i18n/locale/default.json
And then use it like this: this.$t('your-string');

For more information (about for example pluralization or placeholder) you can check out this neat documentation that Mole wrote: https://github.com/JustArchiNET/ASF-ui/tree/main/src/i18n/lib#usage

Thank you very much. I've added the string to localization. The PR should be ready for review now 😀

@Abrynos Abrynos marked this pull request as ready for review June 26, 2021 21:25
@Abrynos Abrynos changed the title [WIP] Implement #1449 Implement #1449 Jun 26, 2021
@Abrynos Abrynos changed the title Implement #1449 Implement #1449 - speed up configuration workflow Jun 26, 2021
@Botan626
Copy link
Contributor

@Abrynos I just had another idea. The dropdown menu could be without checkboxes, but it could allow to multi select property values, until it's closed. Selected values could be highlighted with ui's theme color, and focused values could be highlighted with pale grey color.

And I want to say it again, the less text on screen, the better, look how clean and easy reading interface is on 3d scrrenshot here #1450 (comment), no excessive elements and text.

@Abrynos
Copy link
Member Author

Abrynos commented Jun 28, 2021

I just had another idea. The dropdown menu could be without checkboxes, but it could allow to multi select property values, until it's closed. Selected values could be highlighted with ui's theme color, and focused values could be highlighted with pale grey color.

This further complicates the workflow. I'd like to keep it as simple as possible.

And I want to say it again, the less text on screen, the better, look how clean and easy reading interface is on 3d scrrenshot here #1450 (comment), no excessive elements and text.

I will re-iterate my stance: I want feedback from more than one person. If you are not alone with this opinion I will gladly change it.

@Botan626
Copy link
Contributor

I will re-iterate my stance: I want feedback from more than one person. If you are not alone with this opinion I will gladly change it.

Very odd and obscure for me, why nobody supported this idea in 4 days.

@MrBurrBurr MrBurrBurr linked an issue Jun 28, 2021 that may be closed by this pull request
@MrBurrBurr
Copy link
Member

Thank you @Abrynos, great PR so far!

I gave it a quick review and found some small things that need correction.
Other than that it is looking pretty good 🎉

I will wait for Moles review or do a full review myself once I got more time again.

@Botan626
Copy link
Contributor

Could this new string "Select option" be with faded text at least in order to be less visible, like "All values selected" string now?

image

@Abrynos
Copy link
Member Author

Abrynos commented Jun 29, 2021

@Botan626 interesting idea. I personally liked this one at first glance. This ("All values selected" being greyed out) is due to the whole <select> tag being disabled tho, which we don't want to do (since you wouldn't be able to open the dropdown in that case). Additionally when thinking about it I can see one more problem: It is not possible any more to easily distinguish a disabled dropdown menu from an enabled one any more by just looking at the color.

@Botan626
Copy link
Contributor

@Botan626 interesting idea. I personally liked this one at first glance. This ("All values selected" being greyed out) is due to the whole <select> tag being disabled tho, which we don't want to do (since you wouldn't be able to open the dropdown in that case). Additionally when thinking about it I can see one more problem: It is not possible any more to easily distinguish a disabled dropdown menu from an enabled one any more by just looking at the color.

I didn't want to disable dropdown menu, I'd like to have the "Select option" text to be faded or greyed, to make it less visible, because the more text on screen, the harder to read it.

@Abrynos
Copy link
Member Author

Abrynos commented Jun 29, 2021

I didn't want to disable

Don't worry. I understood that.

@Botan626
Copy link
Contributor

It is not possible any more to easily distinguish a disabled dropdown menu from an enabled one any more by just looking at the color.

I agree, that's why no text is the best choice, gives the most easy reading solution.

@MrBurrBurr MrBurrBurr merged commit 2b42c74 into JustArchiNET:main Jun 29, 2021
@MrBurrBurr
Copy link
Member

Thank you very much for your PR 🎉

@Abrynos Abrynos deleted the feature-speed-up-configuration-workflow branch June 30, 2021 09:24
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 31, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Speed up editing flag configuration properties
3 participants