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

Add support for validation to EuiComboBox and fix some bugs #631

Merged
merged 8 commits into from
Apr 4, 2018

Conversation

cjcenizal
Copy link
Contributor

No description provided.

z-index: $euiZComboBox;
position: absolute;
position: fixed; /* 2 */
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@snide Do you see any problem with this changing to fixed? Is there a reason it needs to be absolute?

Copy link
Contributor

Choose a reason for hiding this comment

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

What happens when you scroll the page? Fixed normally breaks in that situation if it's open unless you are constantly calculating it on scroll.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We update the position as the user scrolls

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like you're constantly calculating so you should be ok. Lemme double check on IE11 though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Little laggy on IE11 but seems acceptable. FWIW it's broken on master in a similar way to the tooltips (where the top value isn't being applied correctly).

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll do some more review tonight, but off-hand I think this works fine. Checked the other browsers as well.

@cjcenizal
Copy link
Contributor Author

@snide Thanks for taking a look. That scroll lag in IE11 is pretty gnarly. I figured out a way to fix the original bug using position: absolute and removing the scroll listener, so hopefully that lag is addressed now.

@snide
Copy link
Contributor

snide commented Apr 4, 2018

@cjcenizal Think that brought in a new issue.

FWIW this looks like a similar problem to the calc on master, where top isn't getting a correct value. On master it just ends up at the bottom of body because it isn't set. Here it looks like it's getting a 0.

@cjcenizal
Copy link
Contributor Author

@snide Is top being set inside the element's style attribute? I just pushed a change which will also apply !important to the inline style, but that shouldn't be necessary since it should have higher specificity than the class already.

@snide
Copy link
Contributor

snide commented Apr 4, 2018

@cjcenizal nope. for some reason your style top value doesn't get calculated. This is the same issue I noticed on #610, so my guess is that it has to do with calculatePopoverPosition but I haven't dug in too deep on your code.

image

@cjcenizal
Copy link
Contributor Author

@snide Thanks, I created #633 to capture this bug so we can address it later.

@cjcenizal cjcenizal merged commit ef67350 into elastic:master Apr 4, 2018
@cjcenizal cjcenizal deleted the combobox-validation branch April 4, 2018 18:27
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.

2 participants