Skip to content

Conversation

@aali309
Copy link
Contributor

@aali309 aali309 commented Nov 29, 2024

Fixes: #20898

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

@bunnyshell
Copy link

bunnyshell bot commented Nov 29, 2024

❌ Preview Environment deleted from Bunnyshell

Available commands (reply to this comment):

  • 🚀 /bns:deploy to deploy the environment

@aali309
Copy link
Contributor Author

aali309 commented Nov 29, 2024

current view

Screenshot 2024-11-29 at 1 08 54 AM

@aali309 aali309 marked this pull request as ready for review November 29, 2024 18:02
@aali309 aali309 requested a review from a team as a code owner November 29, 2024 18:02
@andrii-korotkov-verkada
Copy link
Contributor

I think red-ish background are typically associated with errors. I'd rather add interchanging white and light blue or something like that.

const reset = '\u001b[0m';
const whiteOnYellow = '\u001b[1m\u001b[43;1m\u001b[37m';

const POD_BACKGROUND_COLORS = ['rgba(230, 243, 255, 0.7)', 'rgba(255, 243, 230, 0.7)', 'rgba(230, 255, 230, 0.7)', 'rgba(255, 230, 230, 0.7)', 'rgba(243, 230, 255, 0.7)'];
Copy link
Contributor

Choose a reason for hiding this comment

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

I am wondering if there have to be hard coded or can be gathered from an environment variable ...

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, need to adjust for the dark mode.

Copy link
Member

Choose a reason for hiding this comment

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

you can utilise argo-ui color variables with altreanates to light blue and White in light theme, and light gray and white in dark theme.

return colors[Math.abs(stringHashCode(podName) % colors.length)];
}

const PodLegend: React.FC<{logs: LogEntry[]}> = ({logs}) => {
Copy link
Member

Choose a reason for hiding this comment

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

this would be on top of the log-viewer so in case if there are lot of pods, can have like a Horizontal scroll?

@aali309
Copy link
Contributor Author

aali309 commented Dec 3, 2024

Light Mode:

Screenshot 2024-12-03 at 3 13 26 PM

Dark Mode:

Screenshot 2024-12-03 at 3 11 39 PM

@aali309 aali309 force-pushed the colorLogs branch 2 times, most recently from 8f91c29 to 68f1cc3 Compare December 5, 2024 17:23
@aali309
Copy link
Contributor Author

aali309 commented Dec 9, 2024

@jsoref, how about a dropdown menu to select a pod and then only that selected pod to be coloured? This way we can also colour the logs of that specific pod. This also will get rid of the confusion that may arise with this change when two or more pods have the same colour?

@jsoref
Copy link
Member

jsoref commented Dec 9, 2024

Hmm. We can try that.

@jsoref
Copy link
Member

jsoref commented Dec 9, 2024

I'd probably favor letting people click on a label to the left to switch the actively colored pod.

@aali309
Copy link
Contributor Author

aali309 commented Dec 10, 2024

ok @jsoref @keithchong, I think this is better and less confusing.

  • When a pod is selected, it provides two types of visual indicators:
    Background highlighting of the log entries
    Color change of the pod name (blue in light mode, magenta in dark mode)

  • Unselected pods maintain neutral colors:
    Black text in light mode
    White text in dark mode (for log entries)
    Black text for legend labels regardless of mode

Also, clicking the pod again returns everything back to its default behaviour (no pods colouring or logs highlighting).

Light mode:
Screenshot 2024-12-10 at 2 06 55 PM

Dark mode:
Screenshot 2024-12-10 at 2 07 38 PM

@keithchong
Copy link
Contributor

keithchong commented Dec 10, 2024

This looks better. Just a couple of points:

  • Should there be some indication that there are more pods 'to the right'? It's not clear that the legend is scrollable horizontally (From the css change, it was intentional, so not sure of the reason)
  • The log viewer currently filters the logs by container (the one combo dropdown that is there now), so perhaps the changes now point towards providing a filter for pods too? If you can highlight the log output for one pod, then why not just filter out the others? @jsoref , what do you think? Ultimately, the goal is to isolate output from one pod either by highlighting or by filtering.

@jsoref
Copy link
Member

jsoref commented Dec 18, 2024

No, the goal is not to filter out. It is to highlight while still seeing the other related log entries.

@jsoref
Copy link
Member

jsoref commented Dec 19, 2024

Note: I was expecting to be able to click on a pod name in the log view as opposed to adding a view above....

image

@aali309
Copy link
Contributor Author

aali309 commented Dec 19, 2024

@jsoref, please take a look now. Should work as expected now.

Screenshot 2024-12-19 at 5 15 49 PM

Copy link
Member

@surajyadav1108 surajyadav1108 left a comment

Choose a reason for hiding this comment

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

Overall looks good to me. a small nit I have if it counts as, is that currently if a pod is highlighted with color and then "show pod name" is toggled the color still persist and then user has to go again in "show pod name" to unselect it.

pod-log.mp4

instead we can just unselect the color on toggle for pod name with something like this maybe

    const onToggleViewPodNames = (val: boolean) => {
        setViewPodNamesWithQueryParams(val);
        if (val) {
            setViewTimestampsWithQueryParams(false);
        }else {
            setSelectedPod(null);
        }
    };

If it is intentional then we can keep it as it is

@jsoref
Copy link
Member

jsoref commented Dec 20, 2024

I actually like being able to track a pod through context w/o seeing the pod names -- that's actually incredibly helpful.

You're right that having to show names to clear it is kinda unfortunate. An extra button to reset the highlight or maybe a dropdown to just select a pod to color in the same bar might work -- that'd let someone highlight a pod even if it isn't onscreen so that as soon as it shows up it appears colored -- they might be looking for a pod and want its logging called out. w/ the dropdown the first item in the list could be "no highlight" or something...

@surajyadav1108
Copy link
Member

I actually like being able to track a pod through context w/o seeing the pod names -- that's actually incredibly helpful.

You're right that having to show names to clear it is kinda unfortunate. An extra button to reset the highlight or maybe a dropdown to just select a pod to color in the same bar might work -- that'd let someone highlight a pod even if it isn't onscreen so that as soon as it shows up it appears colored -- they might be looking for a pod and want its logging called out. w/ the dropdown the first item in the list could be "no highlight" or something...

hmm that would be better...

@andrii-korotkov-verkada andrii-korotkov-verkada added the ready-for-review An approver should give a final review and merge the PR label Jan 1, 2025
Copy link
Member

@surajyadav1108 surajyadav1108 left a comment

Choose a reason for hiding this comment

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

LGTM

{isOpen && (
<div className='select-container' style={{position: 'absolute', top: '100%', left: 0, zIndex: 1}}>
<div className='select-options'>
{pods.map(pod => (
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the background or foreground colour need to be changed for dark or light mode?

Copy link
Member

Choose a reason for hiding this comment

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

Fixing pod logs is an item covered in:

Copy link
Contributor

Choose a reason for hiding this comment

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

Could it be fixed as part of this PR, since this is for the new dropdown? (The changes in pod-logs-viewer, also considers the dark mode setting too.)

contrastInDarkMode

Copy link
Member

Choose a reason for hiding this comment

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

There are a whole bunch of colors that are broken. I worry it's a boil the ocean problem. There appear to be at ~5 different "blacks" plus various "whites" and "cyans". I wouldn't even know which colors to suggest.

Offhand, containing shown here is too dark and needs to be lightened. The log pane itself could be true black or one of the various other blacks. As for the text of the log itself, maybe it should be pure white to match the LOGS tab color?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, please take a look @keithchong @jsoref
Screenshot 2025-01-07 at 11 47 39 AM

Copy link
Member

Choose a reason for hiding this comment

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

@aali309 those colors might be ok in dark, they're definitely wrong for light mode.

Copy link
Contributor Author

@aali309 aali309 Jan 7, 2025

Choose a reason for hiding this comment

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

This will be light mode view @jsoref
Screenshot 2025-01-07 at 11 56 32 AM
The dropdown should reflect the correct theme when toggling between light and dark modes.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, I figured out what's going on... it isn't a related bug, it's just a really silly interaction:

Some of the problems @keithchong is describing remind me of:

@keithchong keithchong merged commit 6d27928 into argoproj:master Jan 8, 2025
23 checks passed
@aali309 aali309 deleted the colorLogs branch January 16, 2025 21:29
dudo pushed a commit to dudo/argo-cd that referenced this pull request Jan 18, 2025
* adding background colors for different pods

Signed-off-by: Atif Ali <[email protected]>

* fix lint error

Signed-off-by: Atif Ali <[email protected]>

* fix lint error new

Signed-off-by: Atif Ali <[email protected]>

* resolved issues

Signed-off-by: Atif Ali <[email protected]>

* color name and provide different backround only for selected pod

Signed-off-by: Atif Ali <[email protected]>

* remove pod legend and make pod names clickable on the logs

Signed-off-by: Atif Ali <[email protected]>

* added dropdown to select pods

Signed-off-by: Atif Ali <[email protected]>

* use a marker icon instead to show the dropdown

Signed-off-by: Atif Ali <[email protected]>

* incoorperate darkmode for dropdown

Signed-off-by: Atif Ali <[email protected]>

* Trigger workflow tests

Signed-off-by: Atif Ali <[email protected]>

---------

Signed-off-by: Atif Ali <[email protected]>
Signed-off-by: Brett C. Dudo <[email protected]>
revitalbarletz pushed a commit to revitalbarletz/argo-cd that referenced this pull request Jan 20, 2025
* adding background colors for different pods

Signed-off-by: Atif Ali <[email protected]>

* fix lint error

Signed-off-by: Atif Ali <[email protected]>

* fix lint error new

Signed-off-by: Atif Ali <[email protected]>

* resolved issues

Signed-off-by: Atif Ali <[email protected]>

* color name and provide different backround only for selected pod

Signed-off-by: Atif Ali <[email protected]>

* remove pod legend and make pod names clickable on the logs

Signed-off-by: Atif Ali <[email protected]>

* added dropdown to select pods

Signed-off-by: Atif Ali <[email protected]>

* use a marker icon instead to show the dropdown

Signed-off-by: Atif Ali <[email protected]>

* incoorperate darkmode for dropdown

Signed-off-by: Atif Ali <[email protected]>

* Trigger workflow tests

Signed-off-by: Atif Ali <[email protected]>

---------

Signed-off-by: Atif Ali <[email protected]>
flbla pushed a commit to flbla/argo-cd that referenced this pull request Jan 20, 2025
* adding background colors for different pods

Signed-off-by: Atif Ali <[email protected]>

* fix lint error

Signed-off-by: Atif Ali <[email protected]>

* fix lint error new

Signed-off-by: Atif Ali <[email protected]>

* resolved issues

Signed-off-by: Atif Ali <[email protected]>

* color name and provide different backround only for selected pod

Signed-off-by: Atif Ali <[email protected]>

* remove pod legend and make pod names clickable on the logs

Signed-off-by: Atif Ali <[email protected]>

* added dropdown to select pods

Signed-off-by: Atif Ali <[email protected]>

* use a marker icon instead to show the dropdown

Signed-off-by: Atif Ali <[email protected]>

* incoorperate darkmode for dropdown

Signed-off-by: Atif Ali <[email protected]>

* Trigger workflow tests

Signed-off-by: Atif Ali <[email protected]>

---------

Signed-off-by: Atif Ali <[email protected]>
Signed-off-by: flbla <[email protected]>
vasilegroza pushed a commit to vasilegroza/argo-cd that referenced this pull request Feb 27, 2025
* adding background colors for different pods

Signed-off-by: Atif Ali <[email protected]>

* fix lint error

Signed-off-by: Atif Ali <[email protected]>

* fix lint error new

Signed-off-by: Atif Ali <[email protected]>

* resolved issues

Signed-off-by: Atif Ali <[email protected]>

* color name and provide different backround only for selected pod

Signed-off-by: Atif Ali <[email protected]>

* remove pod legend and make pod names clickable on the logs

Signed-off-by: Atif Ali <[email protected]>

* added dropdown to select pods

Signed-off-by: Atif Ali <[email protected]>

* use a marker icon instead to show the dropdown

Signed-off-by: Atif Ali <[email protected]>

* incoorperate darkmode for dropdown

Signed-off-by: Atif Ali <[email protected]>

* Trigger workflow tests

Signed-off-by: Atif Ali <[email protected]>

---------

Signed-off-by: Atif Ali <[email protected]>
timgriffiths added a commit to timgriffiths/argo-cd that referenced this pull request Dec 29, 2025
PR argoproj#22207 increased line-height from 16px to 1.5rem (24px) to fix
overlapping lines in issue argoproj#22206 after pod highlighting was added.
However, the overlapping only occurs when pod highlighting is active,
so the increased spacing isn't needed when no pod is selected.

This change makes line-height conditional:
- When no pod is highlighted: 1rem (16px) - compact, efficient
- When a pod is highlighted: 1.125rem (18px) - prevents overlap

Benefits:
- Default view is maximally compact (same as original pre-argoproj#22207)
- Only increases spacing when needed (pod highlighting active)
- Uses rem units for accessibility (scales with user font preferences)
- Fixes both issues: no overlap when highlighting, compact when not

This provides the best of both worlds - addressing the overlap issue
from argoproj#22206 while maintaining the compact log view users expect.

Related: argoproj#22206, argoproj#22207, argoproj#21001

Signed-off-by: Timothy Griffiths <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-review An approver should give a final review and merge the PR

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

Offer to color rows by pod

7 participants