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.

Sign up for GitHub

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

Jump to bottom

KEYCLOAK-9387: Add hor scroll & tooltips to role selectors #5866

Merged
merged 1 commit into from
Feb 19, 2019
Merged

KEYCLOAK-9387: Add hor scroll & tooltips to role selectors #5866

merged 1 commit into from
Feb 19, 2019

Conversation

ssilvert
Copy link
Contributor

@ssilvert ssilvert commented Feb 1, 2019

No description provided.

@ssilvert ssilvert self-assigned this Feb 1, 2019
mposolda
mposolda approved these changes Feb 4, 2019
View reviewed changes
Copy link
Contributor

@mposolda mposolda left a comment

Choose a reason for hiding this comment

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

Thanks for the fix!

I left just one comment/question, but it seems it will probably not require any changes in the PR.

For now, I am adding the label "Hold" as AFAIK the master is still not yet opened for fixes (besides test related fixes).

themes/src/main/resources/theme/base/admin/resources/js/services.js
@@ -196,6 +196,13 @@ module.factory('ComponentUtils', function() {

var utils = {};

utils.findIndexById = function(array, id) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just FYI. that there is builtin method "indexOf" - https://www.w3schools.com/jsref/jsref_indexof_array.asp . However it seems it is supported for some newer versions (Especially IE from 9), so it is probably safer to go with our own method? Leaving to you as you probably know better which browsers is admin console supposed to run with.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

indexOf won't work for this. The objects are not actually equal. Only the id is the same. That's why I had to write my own findIndexById method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or to be more specific, the object's elements are the same but it is not the same object instance, so indexOf doesn't work.

Copy link
Contributor

Choose a reason for hiding this comment

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

@ssilvert ah, ok. Thanks!

@mposolda mposolda added the Hold label Feb 4, 2019
@ssilvert ssilvert added Hold and removed Hold labels Feb 6, 2019
@hmlnarik hmlnarik requested a review from pdrozd February 13, 2019 22:32
@ssilvert ssilvert removed the Hold label Feb 14, 2019
@pdrozd
Copy link
Contributor

pdrozd commented Feb 19, 2019

@keycloak-ci-bot test

@keycloak-ci-bot
Copy link

@pdrozd Job is scheduled

@pdrozd pdrozd merged commit 9e16c77 into keycloak:master Feb 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mposolda mposolda mposolda approved these changes

@pdrozd pdrozd pdrozd approved these changes

Assignees

@ssilvert ssilvert

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@ssilvert @pdrozd @keycloak-ci-bot @mposolda