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

Have a check for update button #8189

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

VikalpRusia
Copy link
Contributor

@VikalpRusia VikalpRusia commented Feb 5, 2025

Change

  • Add a "check now" button for updates

Details

How

  1. created a new button
  2. emit an event update-check-now to parent
  3. parent sends ipc event to update/index.ts which is listening to this event
  4. trigger update

Note -> similar structure is there for apply-update

Screenshots

Screenshot 2025-02-06 at 2 06 09 AM

close #7516

@VikalpRusia VikalpRusia force-pushed the check-for-update branch 2 times, most recently from c49a403 to 168b01a Compare February 5, 2025 14:46
Copy link
Contributor

@mook-as mook-as left a comment

Choose a reason for hiding this comment

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

image
I'll let Jan think about the UI instead.

@VikalpRusia VikalpRusia requested a review from mook-as February 5, 2025 19:42
@VikalpRusia VikalpRusia changed the title Draft: Have a check for update button Have a check for update button Feb 5, 2025
@VikalpRusia VikalpRusia marked this pull request as ready for review February 5, 2025 19:45
@jandubois
Copy link
Member

The button height is too tall:

CleanShot 2025-02-05 at 11 53 11@2x

We should match the height of the "Create Snapshot" button:

CleanShot 2025-02-05 at 11 53 59@2x

Or any of the "Troubleshooting" buttons:

CleanShot 2025-02-05 at 11 54 15@2x

You can see that all these buttons also capitalize the second word on the button, so it should say Check Now instead of Check now.

Copy link
Contributor

@mook-as mook-as left a comment

Choose a reason for hiding this comment

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

Please squash your commits; we don't really need to add and remove the excess logging in the commit history. (We use merge commits instead of just squashing, because we often have useful intermediate commits.) It would also be useful to have more detailed commit messages (though in this case that is probably unnecessary).

@VikalpRusia VikalpRusia force-pushed the check-for-update branch 2 times, most recently from 8b7d6a8 to ee1b3ab Compare February 5, 2025 20:33
@VikalpRusia VikalpRusia requested a review from mook-as February 5, 2025 20:35
@VikalpRusia
Copy link
Contributor Author

Please re-trigger the pipeline as error was in the setup phase.

mook-as
mook-as previously approved these changes Feb 6, 2025
Copy link
Contributor

@mook-as mook-as left a comment

Choose a reason for hiding this comment

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

The code looks reasonable. @jandubois please check the UI and merge if you're fine with it.

align-items: center;
}

.btn-xs {
Copy link
Contributor

Choose a reason for hiding this comment

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

That looked too generic, but that's what all the other buttons with limited height do, so I guess that's okay‽

@jandubois
Copy link
Member

jandubois commented Feb 6, 2025

The button looks fine now, but when I click it, nothing happens. It feels like the button doesn't work.

That is of course because there are no updates available. So I think we either need to add a section where we normally display the release notes, that shows the time we have checked, and there are no available updates.

Alternatively we could display a popup dialog that provides the same info, something like this:

CleanShot 2025-02-06 at 15 11 30@2x

But it feels like adding the info to the page itself (and not require the user to press a button in the dialog) is preferable.

We have a similar issue on the Diagnostics page. But there we have a text below the button that will change to "Last run: a few seconds ago" when we click the button:

CleanShot 2025-02-06 at 15 21 55@2x

But that mechanism would look add on the General page, which is not primarily about checking for updates.


I have not tested this, but I wonder what happens if an update is actually available and I press the install button. I assume the update is downloaded, and the app will need to be restarted to install it.

But will this happen even when the "automatically install updates" checkbox is not checked?

@VikalpRusia
Copy link
Contributor Author

Yup popup seems good will work on it

@jandubois
Copy link
Member

Yup popup seems good will work on it

I would prefer to display the text "No updates available" in the space where we would normally show that an update is available, with the release notes.

That way the user doesn't have to click another button to dismiss the popup. This is also more in line with how Other buttons and errors/feedback work in Rancher Desktop (e.g. the "Rerun" button in diagnostics just updates the "last run" field.

@VikalpRusia
Copy link
Contributor Author

I would prefer to display the text "No updates available" in the space where we would normally show that an update is available, with the release notes.

Can we display time also when was it last checked?

Changes
- add new button
- add new event

Signed-off-by: Vikalp Rusia <[email protected]>
@VikalpRusia
Copy link
Contributor Author

VikalpRusia commented Feb 16, 2025

@jandubois - how does it look?
Screenshot 2025-02-17 at 2 34 32 AM

@VikalpRusia VikalpRusia marked this pull request as draft February 16, 2025 21:06
@VikalpRusia
Copy link
Contributor Author

Gentle reminder @jandubois , @mook-as ^^

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.

Offer a button to retry checking for updates
3 participants