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.

Sign up for GitHub

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

Jump to bottom

fix(#18495): Add Alternate color of resources in sync panel #19250

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

fix(#18495): Add Alternate color of resources in sync panel #19250

wants to merge 12 commits into from

Conversation

xyq175com
Copy link

@xyq175com xyq175com commented Jul 26, 2024 •
edited
Loading

Fixes [ISSUE #18495]

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).
  • The title of the PR conforms to the Toolchain Guide
  • 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.
  • 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).
  • My new feature complies with the feature status guidelines.
  • I have added a brief description of why this PR is necessary and/or what this PR solves.
  • Optional. My organization is added to USERS.md.
  • Optional. For bug fixes, I've indicated what older releases this fix should be cherry-picked into (this may or may not happen depending on risk/complexity).

xyq175com and others added 2 commits July 16, 2024 14:15
@xyq175com
add Alternate color of resources in sync panel
f36b98e 
Co-authored-by: Xu Yan <Yan.Xu@fmr.com>
Co-authored-by: Jessie Teng <jessie.teng@fmr.com>
Signed-off-by: Xu, Yan <Yan.Xu@fmr.com>
@xyq175com
format code
992afbb 
Signed-off-by: Xu, Yan <Yan.Xu@fmr.com>
@xyq175com xyq175com requested a review from a team as a code owner July 26, 2024 03:18
@bunnyshell Bunnyshell
Copy link

bunnyshell bot commented Jul 26, 2024 •
edited
Loading

❗ Preview Environment delete from Bunnyshell failed

See: Environment Details | Pipeline Logs

Available commands (reply to this comment):

  • 🚀 /bns:deploy to redeploy the environment
  • ❌ /bns:delete to try again to remove the environment

@bunnyshell Bunnyshell
Copy link

bunnyshell bot commented Jul 26, 2024 •
edited
Loading

❌ Preview Environment deleted from Bunnyshell

Available commands (reply to this comment):

  • 🚀 /bns:deploy to deploy the environment

@xyq175com xyq175com changed the title Fix #18495: Add Alternate color of resources in sync panel Fix: Add Alternate color of resources in sync panel Jul 26, 2024
@xyq175com xyq175com changed the title Fix: Add Alternate color of resources in sync panel Fix(#18495): Add Alternate color of resources in sync panel Jul 26, 2024
@xyq175com xyq175com changed the title Fix(#18495): Add Alternate color of resources in sync panel fix(#18495): Add Alternate color of resources in sync panel Jul 26, 2024
todaywasawesome
todaywasawesome requested changes Jul 29, 2024
View reviewed changes
Copy link
Contributor

@todaywasawesome todaywasawesome left a comment

Choose a reason for hiding this comment

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

Would like to bring this up in the maintainer's meeting to get feedback before merging. Add this "request changes" as a block for that meeting.

rbreeze
rbreeze requested changes Aug 1, 2024
View reviewed changes
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.

This is great. Only concern: how does it look in dark mode? Based on this CSS, I don't think it would adapt

@crenshaw-dev
Copy link
Member

Yeah, dark mode needs work:
image

update style based on maintainer's feedback
b0d924b 
Signed-off-by: Jessie Teng <jessie.teng@fmr.com>
@xyq175com
Copy link
Author

@rbreeze @crenshaw-dev I have updated the code for adoption of dark mode, please help review. Thank you.

@xyq175com
Copy link
Author

@todaywasawesome @rbreeze Please help review when you have time, thank you.

todaywasawesome
todaywasawesome approved these changes Aug 21, 2024
View reviewed changes
Copy link
Contributor

@todaywasawesome todaywasawesome left a comment

Choose a reason for hiding this comment

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

Lgtm!

@todaywasawesome todaywasawesome enabled auto-merge (squash) August 21, 2024 15:17
@xyq175com
Copy link
Author

@rbreeze Can you help review this, thank you!

@xyq175com
Copy link
Author

xyq175com commented Aug 30, 2024 •
edited
Loading

@pasha-codefresh @jannfis when you get a chance wonder if you could help review this PR? Many thanks for your help in advance.

@xyq175com
Copy link
Author

Can someone from this group help review and merge the pr since this pending long time...

@pasha-codefresh
Copy link
Member

@xyq175com me or @reggie-k will review it until EOW

reggie-k
reggie-k suggested changes Sep 4, 2024
View reviewed changes
Copy link
Member

@reggie-k reggie-k 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 good!
The only thing is that for long resource names the green status icon appears out of the line, please see screenshot.

Screenshot 2024-09-04 at 10 05 29

@xyq175com
Copy link
Author

xyq175com commented Sep 4, 2024 •
edited
Loading

It looks good! The only thing is that for long resource names the green status icon appears out of the line, please see screenshot.

Screenshot 2024-09-04 at 10 05 29

Yes make sense, @reggie-k but I dont think this is related to this PR, this PR is address "the Add Alternate color of resources" issue. Could we create another issue for this?

@pasha-codefresh
Copy link
Member

@xyq175com you are right, but the problem that with your changes this issue start to be visible. I would fix it in this PR if it is not super complex

@xyq175com
Add these styles to prevent overflow
f14f8e3 
Signed-off-by: Xu, Yan <Yan.Xu@fmr.com>
@xyq175com
add style base on maintainer feedback
8669e5a 
Signed-off-by: Xu, Yan <Yan.Xu@fmr.com>
auto-merge was automatically disabled September 6, 2024 17:56

Head branch was pushed to by a user without write access

@xyq175com
Copy link
Author

@pasha-codefresh I have updated the fix, please help review. thank you

@xyq175com
Copy link
Author

@pasha-codefresh @reggie-k please help review when you have time, the issue you methioned is been fixed.

@xyq175com
Copy link
Author

Thanks for the reviews @reggie-k , but i still have @rbreeze request changes which block the PR been merged. @rbreeze Can you help review this, this has been blocked for long time.

@pasha-codefresh
Copy link
Member

@rbreeze could you please take a look ?

@xyq175com
Copy link
Author

@rbreeze Can you take a look this?

@xyq175com xyq175com closed this Sep 24, 2024
@xyq175com xyq175com reopened this Sep 24, 2024
@bunnyshell Bunnyshell
Copy link

bunnyshell bot commented Sep 24, 2024 •
edited
Loading

✅ Preview Environment deployed on Bunnyshell

Component Endpoints
argocd https://argocd-kj4rw3.bunnyenv.com/
argocd-ttyd https://argocd-web-cli-kj4rw3.bunnyenv.com/

See: Environment Details | Pipeline Logs

Available commands (reply to this comment):

  • 🔴 /bns:stop to stop the environment
  • 🚀 /bns:deploy to redeploy the environment
  • ❌ /bns:delete to remove the environment

@bunnyshell Bunnyshell
Copy link

bunnyshell bot commented Sep 24, 2024

✅ Preview Environment created on Bunnyshell but will not be auto-deployed

See: Environment Details

Available commands (reply to this comment):

  • 🚀 /bns:deploy to deploy the environment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@todaywasawesome todaywasawesome todaywasawesome approved these changes

@reggie-k reggie-k reggie-k approved these changes

@pasha-codefresh pasha-codefresh pasha-codefresh approved these changes

@rbreeze rbreeze Awaiting requested review from rbreeze

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@xyq175com @crenshaw-dev @pasha-codefresh @todaywasawesome @rbreeze @reggie-k