#376: Refactor Anchor Tags #390
Conversation
| const newUrl = `${window.location.pathname}${window.location.search}${windowLocationHash}`; | ||
| window.history.pushState(null, '', newUrl); | ||
| window.dispatchEvent(new HashChangeEvent('hashchange')); |
There was a problem hiding this comment.
y'all may disagree, but this seems like something that should be handled using React Router, not "manually" with the History API.
thoughts @joneubank, @mistryrn?
There was a problem hiding this comment.
Hard to say as I don't have full context into this project, but ideally I think these would just be plain old anchor tags and we'd take advantage of built-in browser behaviour to achieve the desired functionality.
Does anyone know if there's a reason the team chose to use a button element instead of an anchor on line 198 of AccordionItem.tsx? I see there's something going on with setting clipboard contents as well, maybe that's why.
Again, I lack the full context of this feature/project, so I may just be missing something.
Assuming there is a reason why we're using a button here: I agree that using React Router would be the recommended approach. But I see that this project isn't importing or using React Router at all, so maybe that's why they're doing it "manually"? If there's no application state concerns or re-render concerns, maybe using the History API is fine here versus importing and setting up React Router for just this?
There was a problem hiding this comment.
button element instead of an anchor on line 198 of AccordionItem.tsx? I see there's something going on with setting clipboard contents as well, maybe that's why.
I think you are correct on this assumption, looks like originally it was just clip-boarding. Now its updating the url and clip-boarding.
I think React Router would good to add here as we would be using the React Router in the future.
We are doing work with ERD implementation soon which will require us to implement navigation and would possibly require us to make more customizations onClick beside just navigating(kinda like whats happening with the clipboard).
So it wouldn't be a bad idea to start using something more inline with the React Paradigm than the history api here.
Open to suggestions and insight.
| <TableRow key={row.id} row={row} index={i} /> | ||
| ))} | ||
| {table.getRowModel().rows.map((row, i: number) => { | ||
| const fieldName = (row.original as { name?: string })?.name; |
There was a problem hiding this comment.
Can you type this without casting? As a general rule of thumb, we want to avoid as type casting.
Possibly type guard is needed: https://www.typescriptlang.org/docs/handbook/advanced-types.html
Related to issue: #376
Summary
Refactored several things related to anchor tags, such as fixing schema level anchor tags not updating URL and anchor tags not showing up when hovering over the title text. Also added anchor functionality to field rows, where clicking or routing to to the URL scrolls to the specific field row and highlights it.
Description of Changes
Readiness Checklist
.env.schemafile and documented in the README