-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[UI Framework] Add and Remove state for kui table rows #11020
Conversation
&:hover { | ||
.kuiTableRowHoverReveal { | ||
display: inline-block; | ||
} | ||
} | ||
} | ||
|
||
.kuiTableRow-isDeleted { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Chatted with @snide off-PR. This class name will be changed to kuiTableRow-isBeingDeleted
to reflect more accurately what this style will convey.
@ycombinator addressed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks awesome! I had one question and one suggestion.
Also, do you want to add a button the user can click to execute some JS, which will dynamically apply and remove these state classes? This way people can see the animations in action. Here's how it's done in the Modal example: https://github.com/elastic/kibana/blob/master/ui_framework/doc_site/src/views/modal/modal_example.jsx#L45 and https://github.com/elastic/kibana/blob/master/ui_framework/doc_site/src/views/modal/modal_overlay.js
<td class="kuiTableRowCell kuiTableRowCell--checkBox"> | ||
<div class="kuiTableRowCell__liner"> | ||
<input type="checkbox" class="kuiCheckBox"> | ||
</div> | ||
</td> | ||
<td class="kuiTableRowCell"> | ||
<div class="kuiTableRowCell__liner"> | ||
<a class="kuiLink" href="#">Celebration of some very long content that will affect cell width and should eventually become truncated</a> | ||
<a class="kuiLink" href="#">This is newly added row</a> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small typo: This is a newly added row
background: lighten($errorColor, 35%); | ||
} | ||
|
||
.kuiTableRow-isNew { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been nesting these state classes inside of the root component alongside the pseudo-classes, since they're both used to communicate states. E.g. https://github.com/elastic/kibana/blob/master/ui_framework/components/tabs/_tabs.scss#L54
What do you think? Do you have a preference?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cjcenizal This is why I'd rather use mixins for the css classes, so no preference is allowed :)
I generally don't mind nested classes, but I didn't see much in your code so I kept it out. I'll switch it around if that's the norm.
@snide Following @ycombinator 's comment from my question posting here instead. |
@cjcenizal addressed. If you'd like to add some js for to show the animation in the docs you're welcome to, but it's pretty simple, so i don't think it's needed. Similar to the expanded toggle above it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
We'll need to backport this to 5.x -- ping me if you need a hand with that. |
jenkins, test this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@snide This PR now has conflicts, so those should be addressed before merging (and backporting).
Looks old, closing this out with the expectation that kui -> eui will cover this. Mentioned this in the meta issue. Feel free to reopen if I'm out of date. |
Adds add and remove states for kui table rows based off our error/success variables. Also adds helpful global animation constants that can be used across our sass. The table rows will fade in/out as the classes get applied/removed.
This was a request by @ycombinator