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: support swapping direction of truncation on resource names #8671

Merged

Conversation

reginapizza
Copy link
Contributor

Fixes #8560
This would add a new button to the graph options panel to toggle which direction the title of a resource is going in, allowing them to either see the beginning or end of the names only.

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

@codecov
Copy link

codecov bot commented Mar 3, 2022

Codecov Report

Merging #8671 (afa2ec0) into master (2cdfafe) will decrease coverage by 0.04%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #8671      +/-   ##
==========================================
- Coverage   45.23%   45.18%   -0.05%     
==========================================
  Files         214      214              
  Lines       25420    25420              
==========================================
- Hits        11498    11487      -11     
- Misses      12305    12319      +14     
+ Partials     1617     1614       -3     
Impacted Files Coverage Δ
applicationset/services/scm_provider/github.go 63.52% <0.00%> (-17.65%) ⬇️
util/settings/settings.go 48.10% <0.00%> (ø)
applicationset/services/scm_provider/utils.go 88.50% <0.00%> (+4.59%) ⬆️

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 2cdfafe...afa2ec0. Read the comment docs.

onClick={() => {
toggleTitleDirection();
}}
title={this.state.titleDirection ? 'Title direction right' : 'Title direction left'}>
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if these tooltips are more confusing than helpful. If I hadn't seen the video, the questions these tooltips would raise in my head are: What title? What's a direction in this context?

IMHO, it would be helpful if they are more concise to what will happen, such as "Truncate resource names right" and "Truncate resource names left" - but I'm also not sure on the term "resource names".

Just as something up for discussion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm definitely open to changing it if there's clearer wording, I will switch it to your suggestion for now since it's clearer than what I have at least

@reginapizza reginapizza force-pushed the resourceNameTrunc-issue8560 branch 2 times, most recently from 6ad98cf to 19086a7 Compare March 4, 2022 17:54
Signed-off-by: Regina Scott <rescott@redhat.com>
@keithchong
Copy link
Contributor

keithchong commented Mar 4, 2022

Hey @reginapizza ,

Some general points to consider:

  • Although this does address the original issue description, note that if we do proceed with another solution (eg. eliminate truncation altogether or to address his underlying problem that he wants to see everything all at once), then we should actually undo this change because it will no longer be needed.
  • the new floating toolbar action could be a view preference so that once you set it, it will be set even when you switch to the details view of other apps. Have a look at fix: Make zoom level a user preference (#7183) #8460 as an example of how to do it. If you want, you can contribute another PR for that separately. Keep the above point in mind, if this PR gets merged.

@crenshaw-dev
Copy link
Collaborator

Is anyone actively opposed to this change? We could always give it a try, and if people don't like it, we can try something else.

Copy link
Collaborator

@crenshaw-dev crenshaw-dev left a comment

Choose a reason for hiding this comment

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

I'm a fan. We can always revert. :-)

@crenshaw-dev crenshaw-dev merged commit 60eb2af into argoproj:master Apr 9, 2022
wojtekidd pushed a commit to wojtekidd/argo-cd that referenced this pull request Apr 25, 2022
…oproj#8671)

Signed-off-by: Regina Scott <rescott@redhat.com>

Co-authored-by: Michael Crenshaw <michael@crenshaw.dev>
Signed-off-by: wojtekidd <wojtek.cichon@protonmail.com>
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.

Allow choosing which part (start/end) of a name will get truncated
4 participants