-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
feat(core): add selection for recommended apps #45782
feat(core): add selection for recommended apps #45782
Conversation
I would say yes. |
arrow?!, I meant icon 😅, anyways removed! |
Now looking at the diff I think it can be there can be more solutions for it, like adding another state variable for selected app ids instead of introducing new property in all apps or something similar, in any case pls let me know if any approach is more preferred here |
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.
Very nice! Just some feedback:
- All apps should be pre-checked to be selected, at least the ones which are compatible (which on a release should be all of them)
- If none is selected, the buttons below can be disabled rather than hidden to prevent jumping of the interface
- Checking and unchecking of the box shouldn’t need a spinner, since the installing/downloading is only in the next step? Checking/unchecking should be instant
Nice work @sanskar-soni-9! :)
Thanks @jancborchardt, and checking/unchecking is instant I am using checkbox loading state as an indicator that which app is downloading/installing which I think looks better and cleaner, so I removed previous spinner and installed check icon and overloaded checkbox for every state/step:
I can always bring back previous spinner and check icon if that is needed? |
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.
Awesome, really nice @sanskar-soni-9, and thanks for the explanation! :)
Current UI shift is due to installing status line which get added when apps are installing
I would recommend to put this line next to the "Install recommended apps" button, or better yet change the button to "Installing apps …", cause up there the line is not really visible, and then also the layout does not shift. :)
@jancborchardt done! 😃 |
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.
Looks good design-wise now! :) But has to be checked regarding code.
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.
🚀
Can you rebase onto current master? |
@susnux done :) |
Signed-off-by: Sanskar Soni <sanskarsoni300@gmail.com>
Oops I forgot about this and deleted the repo, new PR in progress |
Summary
Added functionality to selectively install recommended apps
Before
After
2024-06-11.14-45-42.online-video-cutter.com.mp4
Checklist