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

web: Revamp storage proposal page #1138

Merged
merged 31 commits into from
Apr 15, 2024
Merged

web: Revamp storage proposal page #1138

merged 31 commits into from
Apr 15, 2024

Conversation

dgdavid
Copy link
Contributor

@dgdavid dgdavid commented Apr 8, 2024

The Storage Page has become a bit difficult to navigate and a bit overwhelming at first glance, making it more likely to scare users than help them.

The changes in this PR address several weak points we identified: dynamic button/link text, inconsistent widget placement/alignment, and too much information exposed in the first render. As such, the UI proposed is more straightforward, but it is still relevant and helpful for everyone, including advanced and basic users. It keeps contextual help visible and close to the widget/action it belongs to, without diverting user attention from the important bits.

At the time of introducing these changes, the rest of Agama was also taken into consideration so that we could easily maintain consistency by just using the new widgets when needed.

For a better understanding of the changes made, please check commit by commit.


Related to https://trello.com/c/czpTfm3y/3628-5-agama-polish-storage-section (internal link)

@dgdavid dgdavid marked this pull request as draft April 8, 2024 22:32
@dgdavid dgdavid changed the title Next proposal page [WIP] web: Polish storage proposal page. Apr 8, 2024
@coveralls
Copy link

coveralls commented Apr 8, 2024

Coverage Status

coverage: 74.824% (+0.2%) from 74.657%
when pulling bcc4e8a on next-proposal-page
into 766a871 on master.

@lslezak

This comment was marked as resolved.

@ancorgs

This comment was marked as resolved.

@lslezak

This comment was marked as resolved.

@dgdavid
Copy link
Contributor Author

dgdavid commented Apr 9, 2024

@lslezak, @ancorgs

Based on your comments, that's the new proposal:

Partitions collapsed Partitions expanded
Screenshot from 2024-04-09 16-09-38 Screenshot from 2024-04-09 16-09-41

Please, ignore the light green color in the "Partitions..." label ^^^. It's the :hover color.

Snapshots disabled Snapshots enabled
Screenshot from 2024-04-09 15-59-48 Screenshot from 2024-04-09 15-59-51

Please, note that these are cropped screenshots. That's why in some you can see the Result section and in some not.

Click to show/hide other ideas

Just a NP comment: we can even stop mentioning Btrfs in the snapshots label and put it in the explanation.

Screenshot from 2024-04-09 16-00-52

I have not a clear opinion yet, but we could tweak the styles, if needed, for the explanation of these internal fields But later, when every thing is stable. Note below that I simply removed the left margin of the explanation for them. In any case, I prefer consistent whenever possible.

Screenshot from 2024-04-09 15-59-34

@dgdavid dgdavid force-pushed the next-proposal-page branch 2 times, most recently from d4244a4 to 51fd5d9 Compare April 10, 2024 08:08
@@ -215,7 +215,7 @@ const deviceLabel = (device) => {
const deviceChildren = (device) => {
const partitionTableChildren = (partitionTable) => {
const { partitions, unusedSlots } = partitionTable;
const children = partitions.concat(unusedSlots);
const children = partitions.concat(unusedSlots).filter(i => !!i);
Copy link
Contributor Author

@dgdavid dgdavid Apr 10, 2024

Choose a reason for hiding this comment

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

I detected that sometimes this concat resulted in a [partition, partition, ..., undefined]. We should improve it and add a test for this util.

Maybe would be better the proposal below instead of filtering.

- const children = partitions.concat(unusedSlots).filter(i => !!i);
+ const children = unusedSlots ? partitions.concat(unusedSlots) : partitions;

};

describe("SpacePolicyField", () => {
it("renders a button for opening the space policy dialog", async () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO: add a test for the value too, which reveals that maybe a test for SPACE_POLICIES should be added in storage-utils.test.js.

@dgdavid dgdavid force-pushed the next-proposal-page branch 4 times, most recently from 46d1ba3 to 37d70b9 Compare April 10, 2024 16:17
@lslezak

This comment was marked as outdated.

@imobachgs

This comment was marked as outdated.

@lslezak

This comment was marked as outdated.

@lslezak

This comment was marked as outdated.

@ancorgs

This comment was marked as outdated.

@lslezak

This comment was marked as outdated.

A component to help laying out interactive information in pages.
Copy link
Contributor

@ancorgs ancorgs left a comment

Choose a reason for hiding this comment

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

Of course there is a lot of room for improvement, but I see no reason for merging this into master. At least three developers have been playing around with it and we all agree is a clear step forward.

Thus, approving (although this likely deserves a changelog entry).

For following the same strategy than other fields and easily decoupling
the field value from the settings form switch.

The change also removes the internal EncryptionSettingsForm component
in order to reduce the indirection and simplify the code.
These inputs already have labels, which should be enough for identifying
them. Moreover, the aria-label were tied to a "User authentication"
component, which is wrong for a component intended to be used where
needed.
To make a Menu inside a core/Field a bit more compact. In any case,
these styles are going to be polished ASAP.
@dgdavid
Copy link
Contributor Author

dgdavid commented Apr 14, 2024

Great, the current state looks much better! What about this still small simplification?

Current Proposal
agama_storage_result_current agama_storage_result_proposal

Hi @lslezak,

First of all, let me skip the previous proposal since as far as I could see it there was a kind of mis-understanding or lack of sync.

Regarding this one, let me thank you twice 🙏 for the proposal itself and for using the Agama infrastructure to make it possible. No matter whether the proposal is finally adopted or not, playing with Agama to show what you had in mind is hands-down a full win-win since it comes with benefits for all: you, the project, and the team. At least that is what I believe, because it gives you the opportunity to get familiar with the stack, to improve your front-end skills, and even to spot something to fix or improve.

As I said in IRC on Friday, at first sight the proposal LGTM. But still prefer the current status because I feel it could be more meaningful for a broader set of users. To start with, the "During installation" clarification. Probably too obvious for you, and it might seem obvious to a large audience. But IMHO it does not hurt and can help other users. I'm pretty sure there is a lot of for improvements in the UI, but at this moment I think it has a good balance of options and text/inline-help.

In the second place, believe it or not, the "Check all planned actions" was placed there on purpose after quite a few tests and discussions with @ancorgs and @joseivanlopez . In short, there is an intended sequence

  1. Let the user know in a "less disruptive way" the concept of actions and how many will be taken.
  2. Show a "not too much aggressive warning" about destructive actions, if any.
  3. Provide the link to "invite" users to check all the planned actions.

For me still makes sense in that order. Of course, nothing is written in stone and, as said, the UI still has room for improvements here and there. Moreover, proposals like yours are more than welcome as they could come with fresh ideas and a different POV.

About "simplification" I'd like to ask you guys to have no fear of adding needed texts to the UI. I might be wrong, but without neglecting the importance of concision, I do not think we will have a better and cleaner UI by neither reducing texts nor trying to make it fit in "just one screen". We're managing complex concepts and users deserve all the help we can provide: from a fluid layout now that we're delivering a Web product to not hiding explanations when they are needed and not hindering those who do not need them. Storage Proposal is a very good example of a complex part of Agama that, I believe, is now in a shape that doesn't scare and guides the user. What is more, it fits now in the @ancorgs screen 😝 but falling back to an scrollable UI when the user has a smaller screen, more complex setup, or just using a higher zoom level.

Talking about the scroll, I'd like to encourage all the team to embrace it. All modern UI are based on vertical scroll. Timelines, social network app, book apps, and so on. Users are more than used to using scroll and IMHO we should not deny it when laying out Agama pages. It could lead us to make mistakes for a concern that might not have sense in a web UI.

Finally, although I have a lot to learn about UI/UX/a11y/Design, I generally try to consider all kinds of users when working with the Agama UI. As an example, I tend to consider the focus/tab behavior when trying to create a more keyboard friendly user interface. That's might explain why I tend to avoid certain resources or to propose to do something in a different way we are used to.

@dgdavid
Copy link
Contributor Author

dgdavid commented Apr 14, 2024

About "simplification" I'd like to ask you guys to have no fear of adding needed texts to the UI. I might be wrong, but without neglecting the importance of concision, I do not think we will have a better and cleaner UI by neither reducing texts nor trying to make it fit in "just one screen". We're managing complex concepts and users deserve all the help we can provide: from a fluid layout now that we're delivering a Web product to not hiding explanations when they are needed and not hindering those who do not need them. Storage Proposal is a very good example of a complex part of Agama that, I believe, is now in a shape that doesn't scare and guides the user. What is more, it fits now in the @ancorgs screen 😝 but falling back to an scrollable UI when the user has a smaller screen, more complex setup, or just using a higher zoom level.

I forget to add a note about wasting space. Please, do not see explanations, helps, spaces and whatever other resource as a waste of space 🥺 I really don't think that there is too much wasted space currently in Agama. As said multiple times already I might be wrong, but I think that such feeling comes from keeping the mental model that we had in YaST/libyui where saving space all the time is/was something crucial. That's not the case anymore. Of course, we should not waste space just because it's the Web, it's fluid, but in the same way we have to try not to confuse a resource that will help the user in one way or another with such a thing.

@dgdavid
Copy link
Contributor Author

dgdavid commented Apr 14, 2024

Ey @balsa-asanovic 👋

Although I actually do not know how further you are following the Agama development, I'm pinging you here for two reasons:

  • To kindly ask you to check out to the new Storage page.
  • To let you know we're speeding-up the activation of type-checking in the front-end because it helps to deliver a more quality code. This and web: Allow changing file system location #1141 are the latest PR in which we have put more effort on it. I would appreciate it if you could take a look. As you can see, we're relying on JSDoc to do so, although it requires quite more keystokes compared to just using TypeScript which we're not using (yet?) due to historical reasons. Just out of curiosity, would TypeScript be a hindrance to your contributions? Or would be an encouragement instead?

@dgdavid dgdavid changed the title [WIP] web: Polish storage proposal page. web: Revamp storage proposal page Apr 14, 2024
@dgdavid dgdavid marked this pull request as ready for review April 14, 2024 10:02
@dgdavid dgdavid requested a review from ancorgs April 14, 2024 10:06
Copy link
Contributor

@ancorgs ancorgs left a comment

Choose a reason for hiding this comment

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

Better than on Friday. LGTM.

@ancorgs ancorgs merged commit f5946ff into master Apr 15, 2024
1 check passed
@ancorgs ancorgs deleted the next-proposal-page branch April 15, 2024 07:25
dgdavid added a commit that referenced this pull request Apr 15, 2024
At some point in the development of #1138, we realized that even though
it is a native HTML tag, it might not be as accessible as we originally
believed. In fact, the doubt arose when considering keyboard users: How
can they ask the browser to show the title attribute? By default, they
can't.

According to [Adrian
Roselli tests and verdict](https://adrianroselli.com/2024/01/using-abbr-element-with-title-attribute.html),
it is better to stop using them and

  > Explain abbreviations, acronyms, initialisms, numeronyms, etc. on
  > first use and then feel free to fall back to the shortened form.

when really needed, as could be the case of "Full Disk Encryption
(FDE)".
dgdavid added a commit that referenced this pull request Apr 15, 2024
At some point in the development of #1138, we realized that even though
it is a native HTML tag, it might not be as accessible as we originally
believed. In fact, the doubt arose when considering keyboard users: How
can they ask the browser to show the title attribute? By default, they
can't.

According to [Adrian
Roselli tests and verdict](https://adrianroselli.com/2024/01/using-abbr-element-with-title-attribute.html),
it is better to stop using them and

  > Explain abbreviations, acronyms, initialisms, numeronyms, etc. on
  > first use and then feel free to fall back to the shortened form.

when really needed, as could be the case of "Full Disk Encryption
(FDE)".
dgdavid added a commit that referenced this pull request Apr 15, 2024
At some point in the development of #1138, we realized that even though
it is a native HTML tag, it might not be as accessible as we originally
believed. In fact, the doubt arose when considering keyboard users: How
can they ask the browser to show the title attribute? By default, they
can't.

According to [Adrian
Roselli tests and verdict](https://adrianroselli.com/2024/01/using-abbr-element-with-title-attribute.html),
it is better to stop using them and

  > Explain abbreviations, acronyms, initialisms, numeronyms, etc. on
  > first use and then feel free to fall back to the shortened form.

when really needed, as could be the case of "Full Disk Encryption
(FDE)".
dgdavid added a commit that referenced this pull request Apr 15, 2024
Some time ago, we started using the `<abbr>` tag for providing a way to
see the full text of an acronym or abbreviation.

However, at some point in the development of #1138, we realized that
even though it is a native HTML tag, it might not be as accessible as we
originally believed. In fact, the doubt arose when considering keyboard
users: how can they ask the browser to show the title attribute? By
default, they can't.

Having a look to [tests recently made by Adrian
Roselli](https://adrianroselli.com/2024/01/using-abbr-element-with-title-attribute.html),
it can be seen that, among other issues, it's commonly problematic for
keyboard and touch users. In Adrian's words:

> Exposure continues to be inconsistent across browsers and assistive
technologies. Some set of users will always miss some piece of
information.

In addition, such an element increases the risk of overusing it by
encouraging us to over explain acronyms, as @dgdavid almost did with
_Btrfs_, which he wanted to wrap into an `abbr` tag. As other examples,
_XFS_ and _USB_ are acronyms but it isn't actually helpful for readers
to being able to read _Extended File System_ and _Universal Serial Bus_
in the context in which they are used. Similar happens with _LVM_: if
users do not know the technology it represents, reading _Logical Volume
Manager_ hardly can help them.

Thus, it's better to stop using `<abbr />` and evaluate each particular
case when an acronym is added to the UI. If its long version could be
helpful for users we should follow the Adrian recommendation:

> Explain abbreviations, acronyms, initialisms, numeronyms, etc. on
first use and then feel free to fall back to the shortened form.

It could be the case of "Full Disk Encryption (FDE)".
dgdavid added a commit that referenced this pull request Apr 16, 2024
During the presentation of #1138, it was seen that the _shadow_ icon
chosen for some of the actions opening a dialog created confusion in
some attendees when other no-iconized actions opened a dialog too.

Somehow, it was forgotten one of the principles followed in the Agama UI
until now: an action (link, button, button dressed up as a link, menu
item, etc) should be easily recognizable by users as an element they can
interact with in the way they use to do it. I.e., it's not needed to
identify if the action opens a dialog, navigates to another page, to
another section of the same page, opens a menu, shows or hides a
sidebar, etc, since the same action that today opens a dialog in the
future might navigate to another page instead.

Proposed changes here fix the issue by replacing these icons with ones
that represent the concept of the action or by none when it would hamper
more than help. **Keep in mind that icons are merely cosmetic aids for
sight people, reason why the meaning of the actions must not be
determined by them**. The UI must still be meaningful if, for whatever
reason, icons disappear.

Of course, the ones proposed here can be changed at any point when
better suggestions arise. **But remember that at this moment we're
mainly relying on https://fonts.google.com/icons**. We can change our
mind in the future, but choosing a consistent library of icons instead
of grabbing them from here and there. For example, [Lucid
icons](https://lucide.dev/icons/) could be a good candidate for a
library replacement, but in a future **PLEASE**.
@balsa-asanovic
Copy link
Contributor

Ey @balsa-asanovic 👋

Although I actually do not know how further you are following the Agama development, I'm pinging you here for two reasons:

* To kindly ask you to check out to the new Storage page.

* To let you know we're speeding-up the activation of type-checking in the front-end because it helps to deliver a more quality code. This and [web: Allow changing file system location #1141](https://github.com/openSUSE/agama/pull/1141) are the latest PR in which we have put more effort on it. I would appreciate it if you could take a look. As you can see, we're [relying on JSDoc](https://alexharri.com/blog/jsdoc-as-an-alternative-typescript-syntax) to do so, although it requires quite more keystokes compared to just using TypeScript which we're not using (yet?) due to historical reasons. Just out of curiosity, would TypeScript be a hindrance to your contributions? Or would be an encouragement instead?

Hey @dgdavid,

I am still following the project, so thank you for the ping.

I've just checked out the new Storage page and compared it a bit with the old one.
I definitely prefer the new one.
Somehow the design of the sections and their separation is done better.
It is easier to understand what is being done and what to do.

As for the type-checking, kudos for pushing it more. As you know I've worked on a couple of PRs in which the usage of JSDoc was present, so this does not come as a surprise to me.
JSDoc, as you say, seems to be a more verbose choice compared to Typescript, but gives the advantage of not needing to migrate the whole front-end code to Typescript, by merely introducing JSDoc descriptions in the vanilla JavaScript code.

Just out of curiosity, would TypeScript be a hindrance to your contributions? Or would be an encouragement instead?

Definitely not a hindrance. But also, honestly, I wouldn't give it much advantage over the current situation. (at least for me 😄 )

@dgdavid
Copy link
Contributor Author

dgdavid commented Apr 17, 2024

@balsa-asanovic, thanks for the feedback.

@imobachgs imobachgs mentioned this pull request May 17, 2024
imobachgs added a commit that referenced this pull request May 17, 2024
Prepare for releasing Agama 8. It includes the following pull requests:

* #884
* #886
* #914
* #918
* #956
* #957
* #958
* #959
* #960
* #961
* #962
* #963
* #964
* #965
* #966
* #969
* #970
* #976
* #977
* #978
* #979
* #980
* #981
* #983
* #984
* #985
* #986
* #988
* #991
* #992
* #995
* #996
* #997
* #999
* #1003
* #1004
* #1006
* #1007
* #1008
* #1009
* #1010
* #1011
* #1012
* #1014
* #1015
* #1016
* #1017
* #1020
* #1022
* #1023
* #1024
* #1025
* #1027
* #1028
* #1029
* #1030
* #1031
* #1032
* #1033
* #1034
* #1035
* #1036
* #1038
* #1039
* #1041
* #1042
* #1043
* #1045
* #1046
* #1047
* #1048
* #1052
* #1054
* #1056
* #1057
* #1060
* #1061
* #1062
* #1063
* #1064
* #1066
* #1067
* #1068
* #1069
* #1071
* #1072
* #1073
* #1074
* #1075
* #1079
* #1080
* #1081
* #1082
* #1085
* #1086
* #1087
* #1088
* #1089
* #1090
* #1091
* #1092
* #1093
* #1094
* #1095
* #1096
* #1097
* #1098
* #1099
* #1100
* #1102
* #1103
* #1104
* #1105
* #1106
* #1109
* #1110
* #1111
* #1112
* #1114
* #1116
* #1117
* #1118
* #1119
* #1120
* #1121
* #1122
* #1123
* #1125
* #1126
* #1127
* #1128
* #1129
* #1130
* #1131
* #1132
* #1133
* #1134
* #1135
* #1136
* #1138
* #1139
* #1140
* #1141
* #1142
* #1143
* #1144
* #1145
* #1146
* #1147
* #1148
* #1149
* #1151
* #1152
* #1153
* #1154
* #1155
* #1156
* #1157
* #1158
* #1160
* #1161
* #1162
* #1163
* #1164
* #1165
* #1166
* #1167
* #1168
* #1169
* #1170
* #1171
* #1172
* #1173
* #1174
* #1175
* #1177
* #1178
* #1180
* #1181
* #1182
* #1183
* #1184
* #1185
* #1187
* #1188
* #1189
* #1190
* #1191
* #1192
* #1193
* #1194
* #1195
* #1196
* #1198
* #1199
* #1200
* #1201
* #1203
* #1204
* #1205
* #1206
* #1207
* #1208
* #1209
* #1210
* #1211
* #1212
* #1213
* #1214
* #1215
* #1216
* #1217
* #1219
* #1220
* #1221
* #1222
* #1223
* #1224
* #1225
* #1226
* #1227
* #1229
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

Successfully merging this pull request may close these issues.

7 participants