✨ [bento][amp-sidebar] Add check for RTL docs for default side#31821
✨ [bento][amp-sidebar] Add check for RTL docs for default side#31821krdwan merged 7 commits intoampproject:masterfrom
Conversation
0994ceb to
506f3e2
Compare
dvoytenko
left a comment
There was a problem hiding this comment.
Ok. This looks pretty good. Just a couple of comments.
| return; | ||
| } | ||
| setSide(calculateSide(sideProp, sidebarElement.ownerDocument)); | ||
| }, [sideProp, side, mounted]); |
There was a problem hiding this comment.
Do we need mounted here?
There was a problem hiding this comment.
Yes, because on the initial render, mounted = false so we don't get sidebarRef and end up exiting early without doing the setSide calculation. 😕
There was a problem hiding this comment.
Though it's probably worth commenting this fact.
There was a problem hiding this comment.
Just a thought - I'm not convinced we should conditionally render sideBarRef with mounted && <>...</> if we need to work around sidebarRef only sometimes being defined. We now also add hidden styles for sidebar and mask before open anyway, so the conditional render is not even getting us out of doing that.
In the future we will also likely want to always render the content if we want sidebar content to be matchable with content-visibility: hidden-matchable.
However I don't think we should have to do this in this PR.
There was a problem hiding this comment.
I agree this is definitely worth exploring!
| return; | ||
| } | ||
| setSide(isRTL(sidebarElement.ownerDocument) ? Side.RIGHT : Side.LEFT); | ||
| }, [side, mounted]); |
There was a problem hiding this comment.
Only reaches here if sideProp is undefined so we can directly use document.dir to set side
|
|
||
| if (!side) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Hide element directly with hidden attribute on rendered output. (Removed setStyle call)
795279e to
03a88a9
Compare
…oject#31821) * Add check for RTL docs for default side * Review comments * Use state in sidebar * Updated design * Updated sidebar per review comments * Add tests to rtl * Fix a typo
Sidebar tracker: #31366
If
sideattribute / prop is not provided to thesidebarcomponent. The component first checks if the document is an RTL document and if so assigns default side toright, otherwise defaults toleft.todo: add tests