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

Feature/BulkAssignment - Select all #47

Merged
merged 33 commits into from
Jul 21, 2023
Merged

Conversation

petrKavulok
Copy link
Contributor

@petrKavulok petrKavulok commented Jul 6, 2023

in this PR

  • Select all button in Credentials Selection Card
  • Clear selection button in Selected Credentials Card
  • Missing locales update
Screen.Recording.2023-07-07.at.10.28.12.mov

@petrKavulok
Copy link
Contributor Author

Screen.Recording.2023-07-20.at.14.03.43.mov
Screenshot 2023-07-20 at 14 10 34 Screenshot 2023-07-20 at 14 10 45

@petrKavulok petrKavulok requested a review from Pe5h4 July 20, 2023 12:13
@petrKavulok petrKavulok marked this pull request as ready for review July 20, 2023 12:35
Copy link
Collaborator

@Pe5h4 Pe5h4 left a comment

Choose a reason for hiding this comment

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

@petrKavulok Looks good, however I am not sure about the > and < in the cards (the button right next to the credential/tenant). When I see it it makes me a bit confused since I am not sure what those buttons are supposed to do. The + and - would be better

title={t("BulkAssignmentContainer|Select all displayed credentials")}
disabled={allSelected}
>
<span>
Copy link
Collaborator

Choose a reason for hiding this comment

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

@petrKavulok I dont think the span wrapper is necessary here

@Pe5h4 Pe5h4 requested a review from ateska July 20, 2023 14:33
@petrKavulok
Copy link
Contributor Author

@petrKavulok Looks good, however I am not sure about the > and < in the cards (the button right next to the credential/tenant). When I see it it makes me a bit confused since I am not sure what those buttons are supposed to do. The + and - would be better

I agree with you, however these changes were requested, approved and blessed by Aleš

@Pe5h4
Copy link
Collaborator

Pe5h4 commented Jul 20, 2023

@petrKavulok Looks good, however I am not sure about the > and < in the cards (the button right next to the credential/tenant). When I see it it makes me a bit confused since I am not sure what those buttons are supposed to do. The + and - would be better

I agree with you, however these changes were requested, approved and blessed by Aleš

Alrighty

@petrKavulok petrKavulok merged commit b2831c7 into main Jul 21, 2023
@petrKavulok petrKavulok deleted the refactor/bulk-celect-all branch July 21, 2023 14:34
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.

3 participants