report(flow): navigation sidebar #12929
Conversation
Co-authored-by: Patrick Hulce <[email protected]>
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
patrickhulce
left a comment
patrickhulce
left a comment
There was a problem hiding this comment.
nice! great step in the right direction and foundation to build on :)
could you update the PR description as well to represent the direction this shifted.
flow-report/assets/styles.css
Outdated
|
|
||
| .SidebarFlowStep { | ||
| display: flex; | ||
| column-gap: 16px; |
There was a problem hiding this comment.
we're on slightly shakier ground here aren't we? my safari doesn't support column-gap yet for example
There was a problem hiding this comment.
Hmm seems like it should be supported: https://caniuse.com/?search=column-gap
There was a problem hiding this comment.
In 14.1 though, 40% of Safari users won't have column gap due to the lack of evergreen enforcement. I'm not sure we ever officially established our browser support story, but this one seemed easy enough to workaround I'd bring it up.
There was a problem hiding this comment.
Aight workaround seems to be adding a margin to one of the child elements.
|
|
||
| export const SidebarSummary: FunctionComponent = () => { | ||
| const currentLhr = useCurrentLhr(); | ||
| const url = new URL(location.href); |
There was a problem hiding this comment.
note to self, we'll want a utility for setting partial changes to the hash soon :)
flow-report/src/sidebar/flow.tsx
Outdated
| let numSnapshot = 1; | ||
|
|
||
| const steps = flowResult.lhrs.map((lhr, index) => { | ||
| let name; |
There was a problem hiding this comment.
note: we'll want to move this out into an eventual derivedFlowResult object we create that infers a bunch of data like this from the set of LHRs (since multiple components are going to need this), but this is fine for now.
flow-report/src/sidebar/flow.tsx
Outdated
| name = `Snapshot (${numSnapshot++})`; | ||
| break; | ||
| } | ||
| const url = new URL(location.href); |
There was a problem hiding this comment.
we do a lot of a manual URL creation so far. take note of the patterns you see developing so we can coalesce these into some helpers :)
There was a problem hiding this comment.
I think I can remove the manual URL creation and just use the desired hash as the href
flow-report/src/sidebar/flow.tsx
Outdated
| }); | ||
| return ( | ||
| <> | ||
| {steps} |
There was a problem hiding this comment.
just curios, why not map here?
There was a problem hiding this comment.
This was leftover from before we used TSX. Template literal indentation was annoying with eslint and htm so I just extracted it. Inlining here SGTM though.
This comment has been minimized.
This comment has been minimized.
|
@googlebot I consent. |
| "composite": true, | ||
| "outDir": "../.tmp/tsbuildinfo/flow-report", | ||
| "emitDeclarationOnly": true, | ||
| "emitDeclarationOnly": false, |
There was a problem hiding this comment.
@brendankenny this was causing yarn build-report to emit an empty chunk for standalone-flow
There was a problem hiding this comment.
Yeah, really it's only needed if something else is going to consume this as a references dependency and we don't need transpiled js output. Shouldn't be necessary for references and it has real transpiled output anyways :)
5 participants
The
flow-reportdirectory could do with some restructuring, but I can do that later to keep the diff easier to read.A URL param is used to manage the state for the highlighted flow step. For example,
file:///Users/example/report.html?step=1would be the second step in the flow andfile:///Users/example/report.htmlwould be the summary page. In a future PR, we can use something likepreact-routerto manage the URL params instead of doing a full page navigation.