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: Allow opening popups with "stable" size #1156

Merged
merged 5 commits into from
Apr 19, 2024
Merged

Conversation

dgdavid
Copy link
Contributor

@dgdavid dgdavid commented Apr 19, 2024

Although the (ab)use of popup/modal dialog has been avoided as much as possible, they still being a valid resource for some use cases. However, they can become a bit annoying when holding dynamic content and users interactions makes them change their size.

To bypass such a problem, two props has been added to the Agama core/Popup component: blockSize and inlineSize. In a first iteration, both accepts "auto", "small", "medium", and "large" values, setting a stable size on the corresponding axis that it's calculated with respect to the size of viewport (except when using "auto", which actually means no stable size).

As a side effect, in order to have more control on the sizing, the PF/Modal variant prop is being ignored.

Notes for reviewers

  • It makes no sense to add screenshots here. Please, jump into the branch and test it manually if you don't mind.
  • On purpose, this PR does not enable type checking on touched files since changes are more related to CSS than JavaScript.
  • Again, I believe this change does not deserve an entry in the changelog but I happily can add one if you think it is actually needed.

By ignoring the PF/Modal's `variant` prop and adding `blockSize` and
`inlineSize` to core/Popup.

In addition to "auto", they support "small", "medium", and "large"
values that are converted into CSS classes to set the component size
based on the viewport.

The intention is to keep the dialog at a constant size regardless of its
content.

Developers should decide which size is best for a dialog based on its
content and behavior.
CSS still needing much more love, but for now this commit just remove
some dead rules.
@coveralls
Copy link

coveralls commented Apr 19, 2024

Coverage Status

coverage: 74.825% (+0.008%) from 74.817%
when pulling c307876 on add-fixed-popup-sizes
into 85c0ed5 on master.

The default value for inlineSize porp is not "auto" but "medium".
@@ -149,7 +149,8 @@ in the devices listed below. Choose how to do it.");
title={_("Find space")}
description={description}
isOpen={isOpen}
variant="medium"
blockSize="medium"
Copy link
Contributor

Choose a reason for hiding this comment

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

I would use "large" for the blockSize here. In my system only two lines of the table are visible which is not enough to make sense out of the table.

not-enough

Sometimes there are several disks involved at the installation. Or maybe just one disk but with many partitions (bear in mind the list of partitions is expanded automatically when clicking on "custom"). In all those cases, a lot of information is not visible at first sight which makes it harder to understand how to use the whole thing.

If you think "large" is too much, maybe we can use an approach similar to the one in the list of actions, deciding the size based on the number of disks and partitions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you think "large" is too much, maybe we can use an approach similar to the one in the list of actions, deciding the size based on the number of disks and partitions.

I prefer this option, but bearing in mind that it is not so straightforward and I expect more changes in this dialog in the short/mid term, let's go for blockSize="large" as you suggested.

Good catch, BTW.

@ancorgs
Copy link
Contributor

ancorgs commented Apr 19, 2024

I miss the corresponding change in the pop-up to create/edit a storage volume (which is actually one of the most annoying cases). I guess that's because we are reworking that part and you expect the size of that form to become stable. But I wanted to mention it in case it was an overlook.

not-fixed

@dgdavid
Copy link
Contributor Author

dgdavid commented Apr 19, 2024

I miss the corresponding change in the pop-up to create/edit a storage volume (which is actually one of the most annoying cases). I guess that's because we are reworking that part and you expect the size of that form to become stable. But I wanted to mention it in case it was an overlook.

Exactly, I didn't want to touch it here. I should have mentioned it. Thanks for the reminder.

@dgdavid dgdavid requested a review from ancorgs April 19, 2024 09:57
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.

LGTM

@dgdavid dgdavid merged commit 233f29c into master Apr 19, 2024
2 checks passed
@dgdavid dgdavid deleted the add-fixed-popup-sizes branch April 19, 2024 11:45
@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.

4 participants