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: combine form repo settings page #9167 #9374

Merged
merged 2 commits into from Jun 7, 2022

Conversation

reginapizza
Copy link
Contributor

Closes #9167

Checklist:

  • Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
  • The title of the PR states what changed and the related issues number (used for the release note).
  • I've included "Closes [ISSUE #]" or "Fixes [ISSUE #]" in the description to automatically close the associated issue.
  • I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
  • Does this PR require documentation updates? (maybe?)
  • I've updated documentation as required by this PR.
  • Optional. My organization is added to USERS.md.
  • I have signed off all my commits as required by DCO
  • I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
  • My build is green (troubleshooting builds).

This PR will combine the three different methods of connecting repos into one sliding panel with a dropdown to select the connection method.

Video demonstrating new UX flow with HTTPS as an example:

repoSettingsRedesign1.mp4

@codecov
Copy link

codecov bot commented May 11, 2022

Codecov Report

Merging #9374 (e82f4a8) into master (6307b73) will decrease coverage by 0.36%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #9374      +/-   ##
==========================================
- Coverage   46.19%   45.83%   -0.37%     
==========================================
  Files         218      222       +4     
  Lines       25917    26405     +488     
==========================================
+ Hits        11972    12102     +130     
- Misses      12290    12652     +362     
+ Partials     1655     1651       -4     
Impacted Files Coverage Δ
util/argo/managedfields/managed_fields.go 42.04% <0.00%> (-32.96%) ⬇️
util/helm/helm.go 41.89% <0.00%> (-19.45%) ⬇️
...licationset/generators/generator_spec_processor.go 51.66% <0.00%> (-5.91%) ⬇️
applicationset/generators/pull_request.go 39.13% <0.00%> (-3.73%) ⬇️
util/helm/cmd.go 25.29% <0.00%> (-3.53%) ⬇️
applicationset/services/scm_provider/gitlab.go 74.19% <0.00%> (-2.16%) ⬇️
util/session/sessionmanager.go 67.43% <0.00%> (-1.32%) ⬇️
util/cert/cert.go 81.81% <0.00%> (-0.59%) ⬇️
server/application/application.go 31.40% <0.00%> (-0.43%) ⬇️
controller/state.go 73.64% <0.00%> (-0.26%) ⬇️
... and 26 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6307b73...e82f4a8. Read the comment docs.

Copy link
Contributor

@ciiay ciiay left a comment

Choose a reason for hiding this comment

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

It looks great to me! Only left a few small comments. Well done 👍

ui/src/app/settings/components/repos-list/repos-list.tsx Outdated Show resolved Hide resolved
ui/src/app/settings/components/repos-list/repos-list.tsx Outdated Show resolved Hide resolved
@reginapizza reginapizza force-pushed the repoSettingsRedesign branch 2 times, most recently from c080dc9 to ff99ff6 Compare May 24, 2022 03:37
Copy link
Contributor

@ciiay ciiay left a comment

Choose a reason for hiding this comment

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

LGTM

@keithchong
Copy link
Contributor

For this PR, test all three connection types and click on the Connect button with the initial form unchanged.

@reginapizza reginapizza force-pushed the repoSettingsRedesign branch 3 times, most recently from 2b4026d to ee342d1 Compare June 6, 2022 19:13
Signed-off-by: Regina Scott <rescott@redhat.com>
Signed-off-by: Regina Scott <rescott@redhat.com>
@keithchong
Copy link
Contributor

LGTM

@keithchong
Copy link
Contributor

Hi @rbreeze , as an FYI, we've spent some cycles reviewing and testing this, so we'd like to merge this today. If you see something we've missed, we can address them in a separate PR.

@keithchong keithchong merged commit f0a53cd into argoproj:master Jun 7, 2022
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.

Repositories Settings Page Improvements
3 participants