Address post-merge review feedback from PR #74#77
Conversation
- Replace deprecated React.SyntheticEvent with SyntheticEvent named import - Delete dead DocSearch CSS block (~74 lines) introduced in PR #74 - Remove early return guard so feedback FAB renders on all doc pages - Replace manual EditMetaRow markup with @theme/EditMetaRow component - Fix FAB/drawer accessibility: add aria-expanded, aria-controls, tabIndex, aria-hidden, and focus restoration on close; remove spurious aria-modal - Convert DocItemFooter and FeedbackForm to arrow functions per AGENTS.md - Remove inline style by delegating alignment to EditMetaRow CSS module
|
This PR responds to the comments from @felipefdl in this PR: #74 Working on the last comment from that page. |
|
Github Vars added, need to remove unused secrets once deployed. |
felipefdl
left a comment
There was a problem hiding this comment.
Thanks for the follow-up. PR 77 closes the worst landmine (DocSearch CSS conflict with #218) plus a TS error and accessibility gaps, but most of the post-merge review on #74 is still open. Requesting changes so the rest gets addressed before merge.
Fixed in this PR
- #1 DocSearch CSS scope creep (Critical). 74 lines deleted.
- #10 Feature gated on
hasTags || hasEditMeta(Suggestion). Early return removed, FAB now renders on every doc page. - #8 (partial) Hardcoded DocSearch hex colors. Gone with the DocSearch block.
Improvements added beyond my review (nice)
- TypeScript fix:
React.SyntheticEventto namedSyntheticEventimport. - Replaced manual edit-meta markup with
@theme/EditMetaRow. Kills the inlinestyle={{ textAlign: "right" }}. - Arrow-function conversion per AGENTS.md.
- Real a11y upgrades:
aria-expanded,aria-controls,tabIndex,aria-hiddenon the FAB. Removed the bogusaria-modalfrom the non-modal drawer. Focus returned to FAB on close and Escape.
Still open (requesting changes)
Critical
- #2 Silent failure in catch block.
Footer/index.tsx:188-190still hascatch { setStatus("error"); }with no logging. Addconsole.error(err)at minimum so production failures are debuggable. - #3 FAB vs inline-button spec mismatch. PR #74 description said "next to Edit this page" but the implementation is a fixed-position FAB. Either update the spec or move the button inline.
Important
- #4
formKeyremount trick still in place. Reset state explicitly inFeedbackFormviauseEffectkeyed on a prop, or lift state to the parent. - #5
as stringdouble casts. Type guard is not expressed in the type system. Narrow once with an early return. - #6 No fetch timeout. If HubSpot hangs, the form stays in "Sending..." forever. Use
AbortSignal.timeout(10000). - #7 z-index magic numbers (
199/200) collide with the Docusaurus navbar layer. Use a defined scale or document the reason. - #8 (remaining)
#e53e3e(required marker) and#fffstill hardcoded. Replace with--ifm-color-dangerand--ifm-color-white(or equivalent). - #9 Public values treated as repo secrets.
HUBSPOT_PORTAL_ID/HUBSPOT_DOC_GAP_FORM_IDget bundled in client JS and sent in the URL to a public HubSpot endpoint. Move to repo variables (vars.HUBSPOT_PORTAL_ID) instead of secrets in both workflows.
Suggestions
- #11 No tests for a 239-line interactive component (form, observer, escape handler, success timeout, HubSpot submission).
- #12 "Bounces once when the footer first scrolls into view" still resets
visibleon everypermalinkchange, so it bounces on every navigation. Either match the description or update the description.
One new nit introduced by this PR
Footer/index.tsx:75-81 always renders the <footer> (needed for the IntersectionObserver ref) but conditionally applies its classes via clsx. When !hasTags && !hasEditMeta, the DOM ends up with an empty unstyled <footer> element. Functionally fine, semantically odd. Splitting the observer target from the footer element would be cleaner. Not a blocker.
Summary
The PR body says "Closes #74" but it closes roughly 3 of 12 issues from the post-merge review. Happy to approve once the Critical items (#2, #3) and the Important items above are addressed. Tests (#11) can land in a follow-up if scope is a concern.
|
Yes, I'm currently working on the other commit. |
- Separate IntersectionObserver sentinel from <footer>: use a <div
ref={sentinelRef} /> always rendered, render <footer> only when it
has content (tags or edit metadata) — fixes the empty unstyled
<footer> element on pages with neither
- Remove setVisible(false) from permalink effect so the FAB stays
visible across navigation instead of re-triggering the bounce on
every page change
- Replace as-string casts with a proper type guard (feedbackConfig
object, only set when both IDs are strings)
- Remove formKey remount trick; reset form state via useEffect keyed
on the open prop
- Add AbortSignal.timeout(10000) to HubSpot fetch
- Log fetch errors with console.error before setting error status
- Move z-index values to --z-doc-gap-fab / --z-doc-gap-drawer custom properties with a comment documenting the Docusaurus navbar layer (200) - Replace hardcoded #fff and #e53e3e with var(--ifm-color-white) and var(--ifm-color-danger) - Switch HubSpot IDs from secrets to vars in both deploy workflows: these are public client-side values, not credentials
|
Great turnaround on the fixes. 8 of 10 open items closed cleanly. Now fixed
Still open before approval
Keeping the changes-requested status until #3 has a decision. |
Summary
Feedback FAB placement
The feedback UI is a fixed-position FAB in the bottom-right corner, not a button inline with "Edit this page". The FAB appears the first time the bottom of the doc page scrolls into the viewport and remains visible for the rest of the session — it does not re-trigger the entrance animation on every page navigation.
Closes #74