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(ui): allows more convenient sorting of repository. #19829

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

Conversation

bianbbc87
Copy link

@bianbbc87 bianbbc87 commented Sep 7, 2024

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

Fixes: #18454

@bianbbc87 bianbbc87 requested a review from a team as a code owner September 7, 2024 14:19
Copy link

bunnyshell bot commented Sep 7, 2024

🔴 Preview Environment stopped on Bunnyshell

See: Environment Details | Pipeline Logs

Available commands (reply to this comment):

  • 🔵 /bns:start to start the environment
  • 🚀 /bns:deploy to redeploy the environment
  • /bns:delete to remove the environment

Copy link

bunnyshell bot commented Sep 7, 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

@bianbbc87
Copy link
Author

bianbbc87 commented Sep 7, 2024

The ideal sort described in issue was sort, but it was not possible to sort uiwise because Repository objects do not have createdAt properties.
#18454

Instead, among the properties of the repository, it is possible to search by type, project, name, and status.

type: You can search by all, git, and helm.
project : When loading the repository, all projects in the repository array can be listed and searched with the dropdown property.
name : Search statements can be used to search. Remove spaces, ignore case letters
status: You can search using all, Successful, Failed and Unknown.

i deleted sort dropdown

Please let me know if you need anything else.

argo_ui

@bianbbc87 bianbbc87 changed the title feat : allows more convenient sorting of repository. feat(ui) : allows more convenient sorting of repository. Sep 8, 2024
@bianbbc87
Copy link
Author

bianbbc87 commented Sep 8, 2024

@david-wu-octopus

Hi. I referenced your code, but I couldn't sort it in date order by comparing x, y for certain conditions. (Since Repository objects don't have properties to display dates)

Instead, I made it possible to sort using other properties. Especially, I think it's convenient to automatically track and sort projects that you have. Can you please check my code?

@bianbbc87
Copy link
Author

@pasha-codefresh
Hi, can you check my code?

@surajyadav1108
Copy link
Contributor

@bianbbc87

I would suggest few changes:

  1. Change the anchor element from the default argo button to a simpler anchor sort element as same for applications sort method as the button would be confusing

  2. the code breaks for a repo that dosen't have a name property as it was optional for git repos so if user add a git repo through CLI or UI without setting the name property the UI breaks . I tried some changes which I will include and you write on top of it to improve

I have added some changes and it will look something like this :

  • changed the anchor element
  • added some scss for search field as on default it was same as background so "invisible"
  • added some change in name filter(you can improve on it)
changed.mp4

Copy link
Contributor

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

you can add this scss on top of file repo-list.scss for search-bottom

`@import 'node_modules/argo-ui/src/styles/theme';

.search-bar .argo-field{

@include themify($themes) {                  
    border-bottom: 2px solid themed('light-argo-teal-5');;
}

&:focus {
    outline: 0;
    border-color: $argo-color-teal-6;
}

} `

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
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
ui/src/app/settings/components/repos-list/repos-list.tsx Outdated Show resolved Hide resolved
@bianbbc87
Copy link
Author

@bianbbc87

I would suggest few changes:

  1. Change the anchor element from the default argo button to a simpler anchor sort element as same for applications sort method as the button would be confusing
  2. the code breaks for a repo that dosen't have a name property as it was optional for git repos so if user add a git repo through CLI or UI without setting the name property the UI breaks . I tried some changes which I will include and you write on top of it to improve

I have added some changes and it will look something like this :

  • changed the anchor element
  • added some scss for search field as on default it was same as background so "invisible"
  • added some change in name filter(you can improve on it)

changed.mp4

That's very good feedback. Thank you!

bianbbc87 and others added 5 commits September 18, 2024 22:46
Co-authored-by: Suraj yadav <[email protected]>
Signed-off-by: Eunji <[email protected]>
Co-authored-by: Suraj yadav <[email protected]>
Signed-off-by: Eunji <[email protected]>
Co-authored-by: Suraj yadav <[email protected]>
Signed-off-by: Eunji <[email protected]>
Co-authored-by: Suraj yadav <[email protected]>
Signed-off-by: Eunji <[email protected]>
@bianbbc87
Copy link
Author

I’m refactoring it now. 😶

@bianbbc87
Copy link
Author

bianbbc87 commented Sep 19, 2024

@surajyadav1108
I have reflected the modifications, and I have also added some changes below:

  • reduce the width of the search bar to 50%
  • modify the selection to verify the selection by adding the property value to the right of the dropdown

The above two things further strengthened the user experience.
I would appreciate any feedback or suggestions you may have to further improve the implementation. 😃

gif

Copy link
Contributor

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

ok great! @bianbbc87
looking back at it, I noticed that the project has a similar issue with undefined values, just like with the name

undefined.mp4

So I have some few changes as well modify those and then it looks good to me

corrected.mp4

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
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
@bianbbc87
Copy link
Author

@surajyadav1108
Thank you for the feedback! I'm learning a lot 😊

@surajyadav1108
Copy link
Contributor

@surajyadav1108 Thank you for the feedback! I'm learning a lot 😊

ahh seems ui lint check is failing, you can fix it if you have yarn installed globally if not npm install -g yarn

  • cd ui switch to ui
  • yarn install should be 1.22.21 or above
  • yarn run lint -- you can see the errors
  • yarn run lint --fix

bianbbc87 and others added 2 commits September 21, 2024 17:27
@bianbbc87
Copy link
Author

bianbbc87 commented Sep 21, 2024

@surajyadav1108

I've fixed the lint issue.
I think i need a PR label. could you add a PR label?

It's related to ui.

@surajyadav1108
Copy link
Contributor

@surajyadav1108

I've fixed the lint issue. I think i need a PR label. could you add a PR label?

It's related to ui.

No write access, you will have to wait till a member approves it. meanwhile you can fix the PR title

@ishitasequeira can you merge this if it looks good ❓

@sunyeongchoi sunyeongchoi self-assigned this Oct 4, 2024
@bianbbc87 bianbbc87 changed the title feat(ui) : allows more convenient sorting of repository. feat(ui): allows more convenient sorting of repository. Oct 5, 2024
@sunyeongchoi sunyeongchoi added the component:ui User interfaces bugs and enhancements label Oct 13, 2024
@andrii-korotkov-verkada
Copy link
Contributor

@bianbbc87, I've asked for review in CNCF Slack. Can you rebase on the latest master and re-test, please?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:ui User interfaces bugs and enhancements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request: UI: make repository connections page sortable
4 participants