-
Notifications
You must be signed in to change notification settings - Fork 125
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
fix(platform): table empty cell screenreader text #11608
Conversation
✅ Deploy Preview for fundamental-ngx ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Visit the preview URL for this PR (updated for commit 0d81eda): https://fundamental-ngx-gh--pr11608-fix-11596-ng15-tq8ttxeu.web.app (expires Sun, 24 Mar 2024 18:34:55 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 41b993ee8e451bd7c6770b342ce142dc886eacff |
0c24384
to
c092dfd
Compare
@@ -156,7 +156,7 @@ | |||
|
|||
<ng-container *ngIf="row.state === 'readonly'; else editModeCell"> | |||
<span | |||
*ngIf="(row.value | valueByPath : column.key) === '' || (row.value | valueByPath : column.key) === null" | |||
*ngIf="tableTextContainer.innerText.trim() === ''" |
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.
innerText
is undefined
in JSDom, hence issues with unit tests, you either need to fix unit tests with mocking element innerText or do this differently
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.
Plus, relying on object's(tableTextContainer) property in OnPush component will not work always in a best manner. There might be glitches between the detection cycles
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.
"There might be glitches between the detection cycles"
Agreed but this will not be an issue for screenreader users who click or keyboard navigate to this cell, which is what this issue is addressing
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.
Let's take the time and come up with a fix that doesn't affect change detection
fixes #11596