-
-
Notifications
You must be signed in to change notification settings - Fork 488
Adjust library installation dialog buttons style #1401
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer seeing what's on the button instead of ...
(ellipses). We probably need to change the design if the text does not fit the button.
I agree with you @kittaakos, but I used the ellipsis because if we have a very long library name that can't fit the button we still can't see what's in the button. Probably in this case the design needs to be changed.
but I used the ellipsis because if we have a very long library name that can't fit the button we still can't see what's in the button.
Looks like we need some work on the design side. Clearly, having ...
is not something I want to see as a user.
If ...
is accepted as the final design, I am happy to approve the code. Otherwise, we need a better solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is not OK to show ...
. Users cannot see/understand what will happen when clicking on the Install ...
button. See here: #1401 (comment).
If @91volt is OK with it, I am also OK with it. Otherwise, I request changes.
@kittaakos I agree with you that is needed some work on the design side. This means a general check to the cta copy, that shouldn't contain dynamic entities @davegarthsimpson
In this specific case I would fix the issue replacing the second cta as INSTALL WITHOUT DEPENDENCIES
In addition to that I believe we could remove the CANCEL
cta, it is redundant in this case, since we already have the closing icon of the dialog on the top right and the action required of the library installation is not "disruptive".
Since won't be immediate to spot and solve all these occurrences of the text overflow in cta, I believe that we need a rule for those cases, to not break the interface while keeping it usable: I would procede with @francescospissu solution, with the caveat that we need a tooltip with the complete text when it get truncated, the easiest tooltip to implement that I can imagine is the html one given by the title attribute.
I believe that we need a rule for those cases
Use native dialogs, so they won't scale.
91volt
commented
Sep 16, 2022
Use native dialogs, so they won't scale.
@kittaakos I could agree with you but this requires a broader discussion, for what concerns this PR I suggest as follow:
- Adoption of truncation and ellipsis when the text overflow on CTAs, when this happens is needed the title attribute with the complete text
In addition, for this specific dialog:
- Removal of the cancel button
- Substitute the text of the second CTA with:
INSTALL WITHOUT DEPENDENCIES
Please proceed as @91volt has requested. I am discarding my previous rejection.
ubidefeo
commented
Sep 17, 2022
@kittaakos @91volt
the buttons can, IMO, only have
YES / NO / CANCEL
since the question is already asked above them.
"would you like to install all the missing dependencies?"
maybe such text could be enhanced and a note added in an asterisk style or with a circled "?" so the user can hover it and read:
"should you choose not to install dependencies, Sketches using this library might not correctly build and upload"
643e54b
to
cdcf372
Compare
Now it's way better, but if I set the interface scale to the max value (280%) I can't read the whole buttons using at full-screen:
image
The above screenshot was made with the IDE on ~500px. You can also see that the title of the dialog is wrapping to a new line (which I think is inevitable if the name of the library is very long)
As we discussed offline, using the title
attribute to show the whole button ONLY when the button label is partially shown is not so easy (and feels kind of hacky).
@91volt do you think this is enough or can we do something more? If we could find even shorter button labels it would be great, but it's not so easy.
I would at least change the dialog title removing the name of the library (something like "Install library dependencies")
91volt
commented
Oct 12, 2022
I would at least change the dialog title removing the name of the library (something like "Install library dependencies")
I believe this is unrelated to the current issue, since it is not affecting the usability of buttons.
The solution looks good to me, and the case of a 250% scaled up interface keeping the window size smaller than 500px are cases that are not worth to cover IMHO. Is something that is not supported neither by VScode or Replit.
VSCode scaled up interface in small window
Replit scaled up interface in small window
The only thing I can imagine of, is to adopt the title attribute for all buttons, showing its label, this is actually a pattern we can see in a lot of IDE (e.g. VScode buttons).
VScode title attribute buttons
Wdyt? @per1234 @ubidefeo @AlbyIanna
@91volt I totally agree with everything.
If this is the case, I would suggest the following changes for this PR:
- add a
title
attribute to each button - change the title of the dialog to "Install library dependencies"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested it and it works perfectly. Thank you @francescospissu!
LGTM ✅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's use Install All instead of Install all. IDE2 already has one:
arduino-ide/arduino-ide-extension/src/browser/contributions/check-for-updates.ts
Lines 42 to 45 in 87ebcbe
No need to use that label, but please align the casing.
- 🐛 If I click on Install without dependencies nothing happens.
- 🐛 If I click on Install All, only the
Adafruit MAX31855 library
lib is installed, not its dependencies.
203e301
to
d10cd96
Compare
Motivation
When the window size is small, the look of the buttons in the library installation dialog is ugly:
Screenshot 2022年09月05日 114439
Change description
Add ellipsis to prevent text overflow in buttons:
Screenshot 2022年09月05日 114408
Other information
Closes #1314.
Reviewer checklist