Refactor EditableField and FadeText #8858
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
✅ Deploy Preview for ilios-frontend ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
i believe there's a cleaner and simpler solution to the accessibility issue that this is addressing, which builds on most of these changes. please see jrjohnson#896 and #8858. |
stopfstedt
requested changes
Oct 21, 2025
|
|
||
| assert.false(this.expanded, 'text is not expanded'); | ||
| assert.dom('.display-text-wrapper', this.element).hasClass(this.fadedClass); | ||
| assert.ok(component.enabled); |
There was a problem hiding this comment.
needs to await fadedSelector to be present.
Suggested change
| assert.ok(component.enabled); | |
| await waitFor(this.fadedSelector); | |
| assert.ok(component.enabled); |
stopfstedt
previously requested changes
Oct 21, 2025
| import { module, test } from 'qunit'; | ||
| import { setupRenderingTest } from 'test-app/tests/helpers'; | ||
| import { render, waitFor } from '@ember/test-helpers'; | ||
| import { render } from '@ember/test-helpers'; |
There was a problem hiding this comment.
waitFor is still needed.
Suggested change
| import { render } from '@ember/test-helpers'; | |
| import { render, waitFor } from '@ember/test-helpers'; |
Instead of passing options to the component and adding a handler to the container we can use a local modifier to handle the save and close keyboard interactions and pass that back to consumers of EditableField. This makes the API easier to understand as not applying the modifier will prevent any keyboard handing and allows us to remove an invalid interactive issue.
Using named blocks to extract some of the HTML handling from here and remove a few branches. Also removed the unused @showIcon option. WIP commit as more work will need to be done because now there is a text expand button inside the field editing button.
Using contextual components we can curry the values we need for controlling text fading and use them where we apply this component. I also found a way using css mask-image to fade out the text with the component itself instead of needing to do that everywhere we use it. These things combined let us wrap the EditableField inside the fade text so we get the editable behavior, but maintain the expand collapse buttons outside of the editable button. Taken all together we can remove a lot of duplicate code and rely solely on the FadeText component to do this work without needing to duplicate the HTML and styles elsewhere.
This has a tendency to fall out of date (as it did with removing froala and keeping this class here). Instead we can use the modifier directly on the controls as they won't appear in the DOM until they're ready. This takes away some magic and simplifies the component.
When editable field is passed things that look empty it is impossible to click and edit them. I've slightly changed the original behavior and used the icon only if the click prompt isn't passed.
We only use this in one place, but it's nice to have.
Automatically remove links from text, but allow them to be preserved if needed. Can be invoked on the FadeText component or on the yielded contextual component as needed. Mostly we're removing them, but I kept links in when in a read only display of the faded text.
The resizeObserver fires asynchronously in browser code so tests may run before it has a chance to detect the size and send it. This can cause intermittent test failures. I've added a test waiter to FadeText to ensure that we've gotten at least one call to register the size of the element before we test anything.
jrjohnson
force-pushed
the
i6532-faded-bro
branch
from
3574ffe to
7b770d8
Compare
Doing this in the constructor doesn't work because the FadeText is often created with the FadedText hidden.
Doing a lot of test waiter setup and work isn't worth it here. My concern that the non faded text tests were not accurate because they hadn't had a chance to fully render probably isn't worth the extra overhead. Going back to the waitFor check to get these two tests to pass.
|
Adding some thoughts for posterity about the |
stopfstedt
approved these changes
Oct 28, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The purpose was to fix ilios/ilios#6532, but I needed to take the entire thing apart so we could remove links only when needed. Took advantage of contextual component, some things we've learned about modifiers, and a new css feature
mask-imageto remove a lot of duplication.Best reviewed as individual commits.