-
Notifications
You must be signed in to change notification settings - Fork 350
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
Updating Article Renderer to be able to return the focusedElement so that we can scroll these elements into view above the keypad. #755
Conversation
GeraldRequired Reviewers
Don't want to be involved in this pull request? Comment |
npm Snapshot: Published🎉 Good news!! We've packaged up the latest commit from this PR (807202a) and published it to npm. You Example: yarn add @khanacademy/perseus@PR755 |
Size Change: +78 B (0%) Total Size: 855 kB
ℹ️ View Unchanged
|
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #755 +/- ##
==========================================
+ Coverage 59.64% 60.98% +1.33%
==========================================
Files 483 485 +2
Lines 105776 105806 +30
Branches 6057 8765 +2708
==========================================
+ Hits 63092 64521 +1429
+ Misses 42684 41285 -1399
... and 44 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
109b148
to
1c49d4b
Compare
1c49d4b
to
76d5817
Compare
return ( | ||
<DependenciesContext.Provider value={this.props.dependencies}> | ||
<DependenciesContext.Provider | ||
key={i} |
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.
Do we really need to be creating multiple dependencies context providers? It doesn't seem specific to each of these sections at all. However, there could be something I'm missing.
For now, I added a key to stop react from throwing a noisy error about not having unique key props for our children in a list.
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.
CC @jeremywiebe looks like you might have worked on this recently. Was it intentional that we wrapped each Renderer
with DependenciesContext.Provider
? Or could it just wrap all the renderers?
Also I would recommend avoiding using the index as a key. Is there anything from section
that would likely be unique?
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.
No, I don't think we do. We could move it to just wrap the final <div />
that we render these sections into!
// eslint-disable-next-line react/no-string-refs | ||
// @ts-expect-error - TS2339 - Property 'getInputPaths' does not exist on type 'ReactInstance'. | ||
const inputPaths = this.refs[sectionRef].getInputPaths(); | ||
|
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.
String refs are old hat, and now we need a reference to the actual focused element so that we can return it to webapp with the passed in "onFocusChange" method.
didFocusInput = inputPaths.some((inputPath) => { | ||
return Util.inputPathsEqual(inputPath, focusPath); | ||
}); | ||
focusedInput = | ||
this.sectionRenderers[sectionRef].getDOMNodeForPath(focusPath); | ||
} | ||
|
||
if (this.props.apiOptions.onFocusChange != null) { | ||
this.props.apiOptions.onFocusChange( | ||
this._currentFocus, | ||
prevFocusPath, | ||
didFocusInput ? keypadElement?.getDOMNode() : null, |
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.
Making sure we return the focusedInput now, if there is one.
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.
Thanks for investigating this! Glad to see another area where we're removing string refs.
Are there tests we could write for this? For instance a test to make sure ArticleRenderer is calling onFocusChange
with the correct focused element?
I think you're saying that the stories don't scroll into view. Would it be hard to make them actually scroll into view?
export const MultiSectionedExpressionArticle = ({useNewStyles}): any => ( | ||
<TestKeypadContextWrapper> | ||
<KeypadContext.Consumer> | ||
{({keypadElement, setRenderer, scrollableElement}) => ( |
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.
nit: this is my fault because I originally left this in, but lately I've been trying to be better about removing unused context values. scrollableElement
seems unused right now, could we remove it?
const inputPaths = this.refs[sectionRef].getInputPaths(); | ||
|
||
const inputPaths = | ||
this.sectionRenderers[sectionRef].getInputPaths(); |
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.
NAN: just a note on existing code...sectionRef
is confusingly named if it's an index...
@@ -167,13 +172,20 @@ class ArticleRenderer | |||
|
|||
// TODO(alex): Add mobile api functions and pass them down here | |||
const sections = this._sections().map((section, i) => { | |||
const refForSection = `section-${i}`; | |||
const refForSection: any = i; |
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.
Thank you for switching away from string refs!
I'm a little concerned by part of this change. Originally it was a string and now it's a number? Looks like it was used in _handleFocusChange
: [refForSection].concat(newFocusPath)
. Was this an intentional change?
return ( | ||
<DependenciesContext.Provider value={this.props.dependencies}> | ||
<DependenciesContext.Provider | ||
key={i} |
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.
CC @jeremywiebe looks like you might have worked on this recently. Was it intentional that we wrapped each Renderer
with DependenciesContext.Provider
? Or could it just wrap all the renderers?
Also I would recommend avoiding using the index as a key. Is there anything from section
that would likely be unique?
<DependenciesContext.Provider | ||
key={i} | ||
value={this.props.dependencies} | ||
> | ||
<div key={i} className="clearfix"> |
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.
If we find out that we need to wrap each Renderer with the Dependency Provider, do we still need the keys on this div and on the Renderer?
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 thiiiink we're okay for this one as we never remove or reorder sections in Articles. Sadly there's no data in the section that can be used, as it's either that single very very long json string for the Perseus content, or the question answers (that requires digging a little deep). That said, however, I'm totally open to implementing any alternative ideas!
<div key={i} className="clearfix"> | ||
<Renderer | ||
{...section} | ||
ref={refForSection} | ||
ref={(elem) => { | ||
if (elem != null) { |
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.
Nit
if (elem != null) { | |
if (elem) { |
expect(screen.getByRole("textbox")).toBeInTheDocument(); | ||
}); | ||
|
||
it("should call the onFocusChanged callback when an input is focused", async () => { |
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.
Thank you!
this.sectionRenderers[sectionIndex] = elem; | ||
} | ||
}} | ||
key={sectionIndex} |
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.
Do we need this key still now that there's a wrapper with a key instead?
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.
Looks good to me!
…rybook is a little wacky with the keypad and I want to do some proper testing.
a1f2693
to
807202a
Compare
This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## @khanacademy/[email protected] ### Minor Changes - [#773](#773) [`809823e4`](809823e) Thanks [@nixterrimus](https://github.com/nixterrimus)! - Fix text overlap in interactive graph widget - [#755](#755) [`6d9e31a4`](6d9e31a) Thanks [@SonicScrewdriver](https://github.com/SonicScrewdriver)! - Adding logic to ArticleRenderer so that it can return our currently focused element. ## @khanacademy/[email protected] ### Patch Changes - Updated dependencies \[[`809823e4`](809823e), [`6d9e31a4`](6d9e31a)]: - @khanacademy/[email protected]
Summary:
It turns out we never set up our article renderer to return the focused element, which is required to be able to scroll the element above the keypad when the user focuses on it!
This PR adds the missing logic to the article renderer, and also makes a couple of necessary modernizations as we were still using string refs. I've also created a new story for testing multiple inputs in a mobile article. Please note that the keypad does look a little strange in this view as we don't currently have the scrollable element logic in Perseus.
(I suspect that eventually, we should probably move the scrollIntoView logic into Perseus.)