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

feat(core): add selection for recommended apps #45782

Conversation

sanskar-soni-9
Copy link
Contributor

Summary

Added functionality to selectively install recommended apps

Before

240611_14h57m50s_screenshot

After

2024-06-11.14-45-42.online-video-cutter.com.mp4

Checklist

@sanskar-soni-9
Copy link
Contributor Author

I need opinion for this arrow should I remove it?

240611_15h56m45s_screenshot

@sanskar-soni-9 sanskar-soni-9 marked this pull request as draft June 11, 2024 10:49
@AndyScherzinger AndyScherzinger added enhancement 3. to review Waiting for reviews design Design, UI, UX, etc. 2. developing Work in progress feedback-requested and removed 3. to review Waiting for reviews labels Jun 11, 2024
@sanskar-soni-9 sanskar-soni-9 force-pushed the feat/select-install-recommended-apps branch from b8388ac to fc63fa3 Compare June 11, 2024 17:17
@susnux
Copy link
Contributor

susnux commented Jun 11, 2024

I need opinion for this arrow should I remove it?

I would say yes.

@sanskar-soni-9 sanskar-soni-9 force-pushed the feat/select-install-recommended-apps branch from fc63fa3 to e0495cf Compare June 12, 2024 04:23
@sanskar-soni-9
Copy link
Contributor Author

I need opinion for this arrow should I remove it?

arrow?!, I meant icon 😅, anyways removed!

@sanskar-soni-9 sanskar-soni-9 marked this pull request as ready for review June 12, 2024 06:25
@sanskar-soni-9
Copy link
Contributor Author

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

Copy link
Member

@jancborchardt jancborchardt left a 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! :)

@sanskar-soni-9
Copy link
Contributor Author

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:

  1. when user will select app to install checkbox will be blue and enabled
  2. then when he will click on install button then loading checkbox will indicate which app is currently getting installed
  3. and then when app will be installed the checkbox will be disabled and will indicate that the app is installed

I can always bring back previous spinner and check icon if that is needed?

@sanskar-soni-9 sanskar-soni-9 force-pushed the feat/select-install-recommended-apps branch from e0495cf to 018e032 Compare June 15, 2024 11:59
@sanskar-soni-9
Copy link
Contributor Author

Here is the updated UI flow:

2024-06-15.17-13-40.mp4

Current UI shift is due to installing status line which get added when apps are installing
240615_17h17m10s_screenshot

and I still kept loading checkbox because of the previous reason so please let me know your review on this

Copy link
Member

@jancborchardt jancborchardt left a 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. :)

@sanskar-soni-9 sanskar-soni-9 force-pushed the feat/select-install-recommended-apps branch from 018e032 to 8c1b48f Compare June 17, 2024 17:27
@sanskar-soni-9
Copy link
Contributor Author

@jancborchardt done! 😃

@solracsf solracsf added this to the Nextcloud 30 milestone Jun 18, 2024
Copy link
Member

@jancborchardt jancborchardt left a 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.

Copy link
Contributor

@susnux susnux left a comment

Choose a reason for hiding this comment

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

🚀

@susnux
Copy link
Contributor

susnux commented Jun 18, 2024

Can you rebase onto current master?

@sanskar-soni-9 sanskar-soni-9 force-pushed the feat/select-install-recommended-apps branch from 8c1b48f to 5c5cc47 Compare June 19, 2024 02:58
@sanskar-soni-9
Copy link
Contributor Author

@susnux done :)

@sanskar-soni-9 sanskar-soni-9 force-pushed the feat/select-install-recommended-apps branch 2 times, most recently from 53b9c1f to e7b0d1a Compare June 20, 2024 10:13
Signed-off-by: Sanskar Soni <sanskarsoni300@gmail.com>
@sanskar-soni-9 sanskar-soni-9 force-pushed the feat/select-install-recommended-apps branch from e7b0d1a to ddb9122 Compare June 20, 2024 12:49
This was referenced Jul 30, 2024
@sanskar-soni-9 sanskar-soni-9 closed this by deleting the head repository Aug 4, 2024
@sanskar-soni-9
Copy link
Contributor Author

Oops I forgot about this and deleted the repo, new PR in progress

@skjnldsv skjnldsv removed this from the Nextcloud 30 milestone Aug 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Check/Uncheck button to install specific recommended app from "/apps/recommended" page
6 participants