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: show sync wave in ui #8902

Merged
merged 18 commits into from
Apr 28, 2022

Conversation

ciiay
Copy link
Contributor

@ciiay ciiay commented Mar 25, 2022

Signed off: yicai@redhat.com
Closes: #8598

Note on DCO:

If the DCO action in the integration test fails, one or more of your commits are not signed off. Please click on the Details link next to the DCO action for instructions on how to resolve this.

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?
  • 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).

Screenshot(updated):
Screen Shot 2022-04-26 at 2 19 32 PM

@ciiay ciiay force-pushed the ui-8598-show-sync-wave-in-ui branch from 7cebca6 to c58fea3 Compare March 25, 2022 20:11
@codecov
Copy link

codecov bot commented Mar 25, 2022

Codecov Report

Merging #8902 (038fd98) into master (5f0e58d) will decrease coverage by 0.00%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #8902      +/-   ##
==========================================
- Coverage   45.80%   45.79%   -0.01%     
==========================================
  Files         219      219              
  Lines       25864    25864              
==========================================
- Hits        11846    11845       -1     
- Misses      12372    12373       +1     
  Partials     1646     1646              
Impacted Files Coverage Δ
applicationset/services/scm_provider/github.go 75.29% <0.00%> (-5.89%) ⬇️
util/settings/settings.go 48.10% <0.00%> (+0.46%) ⬆️

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 5f0e58d...038fd98. Read the comment docs.

@ciiay ciiay force-pushed the ui-8598-show-sync-wave-in-ui branch 2 times, most recently from 65e7252 to 31b3936 Compare March 29, 2022 01:29
@ciiay ciiay requested a review from terrytangyuan March 29, 2022 13:18
@ciiay ciiay force-pushed the ui-8598-show-sync-wave-in-ui branch 2 times, most recently from 7444ab5 to 6cef024 Compare March 29, 2022 19:41
@ciiay ciiay requested a review from terrytangyuan March 29, 2022 19:55
@ciiay ciiay force-pushed the ui-8598-show-sync-wave-in-ui branch from 6f881ee to a785dca Compare April 7, 2022 20:28
@ciiay ciiay requested a review from keithchong April 7, 2022 21:44
@ciiay ciiay force-pushed the ui-8598-show-sync-wave-in-ui branch 3 times, most recently from 99f4842 to 325de3d Compare April 12, 2022 18:28
@ciiay
Copy link
Contributor Author

ciiay commented Apr 12, 2022

Hi @rbreeze , could you take a look at this PR? Thanks!

Copy link
Member

@rbreeze rbreeze 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, just one small request

@ciiay ciiay force-pushed the ui-8598-show-sync-wave-in-ui branch from 1831305 to 7cc812c Compare April 21, 2022 18:13
@ciiay ciiay requested a review from rbreeze April 21, 2022 18:14
@ciiay ciiay force-pushed the ui-8598-show-sync-wave-in-ui branch 2 times, most recently from 40fcedc to a532713 Compare April 21, 2022 19:23
@pasha-codefresh
Copy link
Member

pasha-codefresh commented Apr 21, 2022

@ciiay

First of all great job , i think in terms of ux the fact that column can contain or not contain sync order column is not good, you can see in screenshot same app table but different pages
I would suggest check if app contains sync waves show column otherwise no
Снимок экрана 2022-04-21 в 22 22 09
Снимок экрана 2022-04-21 в 22 22 16

@ciiay
Copy link
Contributor Author

ciiay commented Apr 23, 2022

@ciiay

First of all great job , i think in terms of ux the fact that column can contain or not contain sync order column is not good, you can see in screenshot same app table but different pages I would suggest check if app contains sync waves show column otherwise no Снимок экрана 2022-04-21 в 22 22 09 Снимок экрана 2022-04-21 в 22 22 16

Hi @pasha-codefresh , thanks for the detailed feedback. I learned a lot from it. Regarding this observation, previously I got a feedback that it looked weird when there was a Sync Order column while no values show up underneath it. So I added the check to each page of resources. I'm not sure about the best solution to this specific situation. Anymore thoughts about this?

Reference: ui/src/app/applications/components/application-details/application-resource-list.tsx line 20 - line 31

@ciiay ciiay force-pushed the ui-8598-show-sync-wave-in-ui branch from 2fb1c72 to 3d8f86a Compare April 23, 2022 02:19
@ciiay ciiay requested a review from pasha-codefresh April 25, 2022 20:51
@keithchong
Copy link
Contributor

Regarding the inconsistency in showing or not showing the column from page to page, I agree with @pasha-codefresh. If we always show it, then perhaps instead of having the column value blank, maybe show a dash/hyphen '-' ?

On the other hand, if we don't show the column if none of the resources have the sync waves/order, what will happen if a sync order is added to one of the resources, while you are on one of the pages? If you refresh, then the column will be added? Is that expected?

@ciiay
Copy link
Contributor Author

ciiay commented Apr 26, 2022

Regarding the inconsistency in showing or not showing the column from page to page, I agree with @pasha-codefresh. If we always show it, then perhaps instead of having the column value blank, maybe show a dash/hyphen '-' ?

On the other hand, if we don't show the column if none of the resources have the sync waves/order, what will happen if a sync order is added to one of the resources, while you are on one of the pages? If you refresh, then the column will be added? Is that expected?

@keithchong Thanks for the suggestion. I agree it's better to keep it consistent, even when none of the resources has Sync Order information, we should keep the column just to make it consistent everywhere and every time the user opens the list view. I updated the code with displaying hyphen on empty Sync Order value. @pasha-codefresh How do you think about this solution?

How it looks when None resources has Sync Order value:

Screen Shot 2022-04-26 at 2 19 20 PM

How it looks when some of the resources have Sync Order value:

Screen Shot 2022-04-26 at 2 19 32 PM

ciiay added 16 commits April 26, 2022 18:50
Signed-off-by: ciiay <yicai@redhat.com>
Signed-off-by: ciiay <yicai@redhat.com>
Signed-off-by: ciiay <yicai@redhat.com>
Signed-off-by: ciiay <yicai@redhat.com>
Signed-off-by: ciiay <yicai@redhat.com>
Signed-off-by: ciiay <yicai@redhat.com>
Signed-off-by: ciiay <yicai@redhat.com>
Signed-off-by: ciiay <yicai@redhat.com>
Signed-off-by: ciiay <yicai@redhat.com>
Signed-off-by: ciiay <yicai@redhat.com>
Signed-off-by: ciiay <yicai@redhat.com>
Signed-off-by: ciiay <yicai@redhat.com>
Signed-off-by: ciiay <yicai@redhat.com>
Signed-off-by: ciiay <yicai@redhat.com>
Signed-off-by: ciiay <yicai@redhat.com>
Signed-off-by: ciiay <yicai@redhat.com>
@ciiay ciiay force-pushed the ui-8598-show-sync-wave-in-ui branch from e402950 to c0f26c0 Compare April 26, 2022 22:50
Copy link
Contributor

@keithchong keithchong left a comment

Choose a reason for hiding this comment

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

LGTM too, and Yi addressed the outstanding review comments.

@keithchong keithchong enabled auto-merge (squash) April 27, 2022 19:04
@keithchong keithchong disabled auto-merge April 27, 2022 19:05
@keithchong keithchong enabled auto-merge (squash) April 27, 2022 19:07
Copy link
Member

@pasha-codefresh pasha-codefresh left a comment

Choose a reason for hiding this comment

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

Great job

Copy link
Member

@rbreeze rbreeze left a comment

Choose a reason for hiding this comment

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

LGTM!

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.

Show sync wave order in UI
5 participants