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

Expose refreshInterval input field for cluster repos #12759

Merged
merged 9 commits into from
Dec 12, 2024

Conversation

momesgin
Copy link
Member

@momesgin momesgin commented Dec 3, 2024

Summary

Fixes #12486

Occurred changes and/or fixed issues

New labeled input added to the cluster repo page to let users to change the refreshInterval for their cluster repos.

Areas or cases that should be tested

Test the create/edit modes with/without changing the refreshInterval

Areas which could experience regressions

n/a

Screenshot/Video

refreshinterval.mov

Checklist

  • The PR is linked to an issue and the linked issue has a Milestone, or no issue is needed
  • The PR has a Milestone
  • The PR template has been filled out
  • The PR has been self reviewed
  • The PR has a reviewer assigned
  • The PR has automated tests or clear instructions for manual tests and the linked issue has appropriate QA labels, or tests are not needed
  • The PR has reviewed with UX and tested in light and dark mode, or there are no UX changes

@momesgin momesgin added this to the v2.11.0 milestone Dec 3, 2024
@momesgin momesgin self-assigned this Dec 3, 2024
@edenhernandez-suse
Copy link

In terms of consistency (although I don't love the component itself), we probably should use the input field with unit:

image

If not, at least I would change the label and placeholder to:

  • Label: Refresh interval (hours)
  • Placeholder: default: 6 hours

And, ideally, it could just sit inside the "Target" section without its own header, since it's just a setting related to the repo target (how often it's fetched).

@kwwii are you ok with this?

@kwwii
Copy link
Contributor

kwwii commented Dec 6, 2024

@edenhernandez-suse Yes, I agree with using the component we have now for showing the units as suggested.

@momesgin
Copy link
Member Author

momesgin commented Dec 6, 2024

@kwwii @edenhernandez-suse thank you for reviewing, so does this mean we should update the OCI inputs as well to use the same unit input component for consistency?

To be more clear, only two of the three input fields for OCI are in seconds, the number of retries input is just a number.

@momesgin momesgin marked this pull request as draft December 6, 2024 18:20
@edenhernandez-suse
Copy link

Sorry @momesgin I didn't see the OCI options at the end of the video (in these cases I think having screenshots is a bit better to ensure we don't miss anything).

Yes, if a field has units, ideally it should use the component with units. The number of retries has no units so that's fine, a regular input type number is perfect. However, the components with units are huge (we should redesign them at some point), so the layout might break. Please ping me in Slack or here if that happens and I'll try to provide a solution.

@momesgin
Copy link
Member Author

Sorry @momesgin I didn't see the OCI options at the end of the video (in these cases I think having screenshots is a bit better to ensure we don't miss anything).

Yes, if a field has units, ideally it should use the component with units. The number of retries has no units so that's fine, a regular input type number is perfect. However, the components with units are huge (we should redesign them at some point), so the layout might break. Please ping me in Slack or here if that happens and I'll try to provide a solution.

No worries at all @edenhernandez-suse , I had a meeting with Ken yesterday re this issue. I'm currently working on fixing e2e tests after switching to the UnitInput component, I'll reach out either here or on Slack once I'm finished with that.

@momesgin
Copy link
Member Author

@edenhernandez-suse here is the final result:

image image image

please let me know if anything needs to be changed

@edenhernandez-suse
Copy link

All good!

codyrancher
codyrancher previously approved these changes Dec 12, 2024
@codyrancher codyrancher self-requested a review December 12, 2024 19:02
@momesgin momesgin marked this pull request as ready for review December 12, 2024 19:04
@momesgin momesgin merged commit 7c24ad8 into rancher:master Dec 12, 2024
29 checks passed
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.

Add UI option for refreshInterval on a Helm Repository
4 participants