chore: Convert component to web component and update build workflows #47
Conversation
|
mrderyk
force-pushed
the
chore/web_component
branch
3 times, most recently
from
3c89f5a to
f08d9b5
Compare
mrderyk
changed the title
There was a problem hiding this comment.
I tested the preview by connecting the component to my own corpus and it worked great. I found a regression which I noted in my comments below.
I also found one bug with the demo site, which I don't consider blocking since it's not a regression. When I toggle the "Open results in new tab" option, it doesn't actually cause clicking on the search results to open them in a new tab.
|
|
||
| - name: Build react-search Package | ||
| run: npm run build | ||
| - name: Install and Build 🔧 # This example project is built using npm and outputs the result to the 'build' folder. Replace with the commands required to build your project, or remove this step entirely if your site is pre-built. |
There was a problem hiding this comment.
Is this comment relevant? Looks like copy-paste from an example, so if it's still helpful maybe we can reword it, e.g. refer to this project instead of an example project?
src/SearchModal.tsx
Outdated
| @@ -37,20 +41,132 @@ export const SearchModal = forwardRef(({ onClose, isOpen, children }: Props, ref | |||
| onEscapeKey={onCloseDelayed} | |||
| onClickOutside={onCloseDelayed} | |||
| // Enable manual focus return to work. | |||
| returnFocus={false} | |||
| returnFocus={true} | |||
There was a problem hiding this comment.
Is this supposed to return focus to the button when the modal is closed? I noticed that this isn't working in this PR, but it is working in master. So I think there's a regression regarding focus in this PR, potentially related to this change.
There was a problem hiding this comment.
Oops I was trying something out here and forgot to take it out. Good catch!
mrderyk
force-pushed
the
chore/web_component
branch
from
f08d9b5 to
a71ff14
Compare
|
@cjcenizal I fixed the regression. The root cause was a gotcha in the naming convention of the web component's observable attributes- they are all internally converted to lower case. I handled this in react-chat, but forgot to here. I put a comment in the code after fixing this issue, so future maintainers are aware of the problem. |
|
When I run |
| // We have to be more specific when picking out the return focus element. | ||
| // This is because document.activeElement is now a custom element containing a shadow DOM | ||
| // In order to properly return focus, we key in on the actual button inside the shadow DOM. | ||
| returnFocusElRef.current = document.activeElement?.shadowRoot?.querySelector("button") as HTMLElement; |
There was a problem hiding this comment.
Nit: Maybe use getElementById to truly future-proof this, in case we add another button? By using a semantic name for the ID this might also become more readable.
CONTEXT
In order to avoid further CSS conflict issues (and therefore cast a wider compatibility net for our search component), we should mount search in a web component.
CHANGES