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(bulk-import): add permission checks in bulk import UI #2034

Merged
merged 1 commit into from
Aug 28, 2024

Conversation

debsmita1
Copy link
Member

@debsmita1 debsmita1 commented Aug 9, 2024

Copy link
Contributor

@ciiay ciiay left a comment

Choose a reason for hiding this comment

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

Hi @debsmita1 , thanks for the PR 👏 Looks like bottom button margin is a bit off.
image

  • If it's admin or user with permission, it should display the Edit icon button instead of the External Link icon button on the Added repositories row.
  • No loading indicator on deleting a row from Added repositories. Maybe we can improve this behavior? Didn't notice this problem until actually tested out.
pr_bulk_import_permission.mp4

@debsmita1
Copy link
Member Author

Hi @debsmita1 , thanks for the PR 👏 Looks like bottom button margin is a bit off. image

Fixed
Screenshot 2024-08-14 at 11 29 23 AM

  • If it's admin or user with permission, it should display the Edit icon button instead of the External Link icon button on the Added repositories row.

Still working on this

  • No loading indicator on deleting a row from Added repositories. Maybe we can improve this behavior? Didn't notice this problem until actually tested out.

Showing this spinner before the Button

Screenshot 2024-08-14 at 11 28 48 AM

pr_bulk_import_permission.mp4

@ciiay
Copy link
Contributor

ciiay commented Aug 15, 2024

Hi @debsmita1 the button margin and delete spinner work great.

Not sure if it's in scope of this PR, but when I added a repository, it didn't show up in the Added repositories table.

bulk_import_pr_review.mp4

@ciiay
Copy link
Contributor

ciiay commented Aug 22, 2024

Hi @debsmita1 , can you take a look at the Edit action? Icon flashes between edit icon and external link icon.

bulk_import_rbac_review.mp4

Copy link
Contributor

@ciiay ciiay left a comment

Choose a reason for hiding this comment

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

Works great 👏 Only a small change request. The Organization URL should be trimmed too.
image

@debsmita1
Copy link
Member Author

Works great 👏 Only a small change request. The Organization URL should be trimmed too. image

Fixed
Screenshot 2024-08-28 at 11 30 43 AM

@openshift-ci openshift-ci bot removed the lgtm label Aug 28, 2024
Copy link

Copy link
Member

@invincibleJai invincibleJai left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Aug 28, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit e0f753f into janus-idp:main Aug 28, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants