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

couple UI fixes for users management #17082

Merged
merged 10 commits into from Jan 22, 2021
Merged

Conversation

diosmosis
Copy link
Member

Description:

Contains the following fixes:

  • the logic used to enable the bulk actions dropdown based on selected row count was not behaving correctly, since the model of checkboxes was being updated after the ng-click fired. fixed w/ a $timeout.
  • fix overlapping selects using an event available in materializecss. selects would overlap because they had the same z-index as each other. we used to try and change the z-indexes on focus and on blur, but blur is invoked right after focus. the materializecss event, however, is fired at the right time.
  • since one user has trouble viewing a certain unicode character, switched the 2FA column in the users management table to use an icon. might be confusing since the icon is also used for actions, elsewhere.

Fixes #17079

Review

  • Functional review done
  • Usability review done (is anything maybe unclear or think about anything that would cause people to reach out to support)
  • Security review done see checklist
  • Code review done
  • Tests were added if useful/possible
  • Reviewed for breaking changes
  • Developer changelog updated if needed
  • Documentation added if needed
  • Existing documentation updated if needed

@diosmosis diosmosis added the Needs Review PRs that need a code review label Jan 13, 2021
@diosmosis diosmosis added this to the 4.2.0 milestone Jan 13, 2021
@diosmosis diosmosis added the not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. label Jan 13, 2021
@sgiehl
Copy link
Member

sgiehl commented Jan 13, 2021

The role dropdown still looks broken in chrome:
image

This seems to be caused by the transform: scale(0.8) on the select-wrapper. Disabling it fixes the issue. Guess the reason it that transform creates a new stacking context for z-index

@diosmosis
Copy link
Member Author

@sgiehl odd, it was working for me. I'll look again.

@diosmosis
Copy link
Member Author

@sgiehl i'm not able to reproduce:

image

image

actually... was just able to reproduce. it only happens if you click on another select while one is open. Will fix by removing transform, much simple solution.

@sgiehl
Copy link
Member

sgiehl commented Jan 14, 2021

That fix works for me on chrome. But shouldn't we make the font-size in the drop down smaller as well. Guess that was the case before (due to the transform of the parent element)

Also I just found another issue in the user management. The role drop down is also showing the capabilities. Is that on purpose? Selecting one of the capabilities even doesn't add it for the user.

@diosmosis
Copy link
Member Author

That fix works for me on chrome. But shouldn't we make the font-size in the drop down smaller as well. Guess that was the case before (due to the transform of the parent element)

I'm not sure it's really needed. The transform was just so it didn't stretch out the rows in the table. Personally I don't think it really matters the dropdown font is different. I can change it if needed, though.

@sgiehl
Copy link
Member

sgiehl commented Jan 18, 2021

@diosmosis we can also keep it that way. But imho the drop down content looks a bit oversized, compared to the other elements.

image

@diosmosis
Copy link
Member Author

@sgiehl updated the dropdown text height:

image

@sgiehl
Copy link
Member

sgiehl commented Jan 20, 2021

@diosmosis looks better now. Would be good to merge from my side. But there are a couple of UI tests failing due to this changes...

@diosmosis diosmosis merged commit cc25c84 into 4.x-dev Jan 22, 2021
@diosmosis diosmosis deleted the 17079-users-manager-fixes branch January 22, 2021 05:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review PRs that need a code review not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Editing Users screen is partially broken
2 participants