-
Notifications
You must be signed in to change notification settings - Fork 100
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
refactor(ui-dom-utils): update findFocusable
logic
#1223
refactor(ui-dom-utils): update findFocusable
logic
#1223
Conversation
instructure-ui-ci seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
findFocusable
logicfindFocusable
logic
Preview URL: https://1223--preview-instui.netlify.app |
@@ -83,12 +83,7 @@ function findFocusable( | |||
|
|||
function hidden(element: Element | Node) { | |||
const cs = getComputedStyle(element) | |||
return ( |
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 refers to a ticket on the Slack channel. After researching the issue, I discovered that the developers who were implementing the feature had left. Therefore, I decided to remove the feature and test how it affected the components. Overall, everything was fine in the manual test. I suggest writing unit tests.
ce51a7a
to
01e4ef8
Compare
…display is 'none' The code of findFocusable was lifted from some ancient JQuery UI helper. For some unknown reason it treats an element as non-focusable if its CSS display prop is not inline and its size is 0 or less. It seems to work fine without this criteria, and we could not find any reason why this clause was added in the first place.
01e4ef8
to
c41a3ee
Compare
The code of It seems to work fine without this second criteria, and we could not find any reason why this clause was added in the first place (even when looking at JQuery UI's commit log). |
No description provided.