-
Notifications
You must be signed in to change notification settings - Fork 9.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm seems like it should be supported: https://caniuse.com/?search=column-gap
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just curios, why not map here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
@@ -2,7 +2,7 @@ | |||
"compilerOptions": { | |||
"composite": true, | |||
"outDir": "../.tmp/tsbuildinfo/flow-report", | |||
"emitDeclarationOnly": true, | |||
"emitDeclarationOnly": false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@brendankenny this was causing yarn build-report
to emit an empty chunk for standalone-flow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 :)
The
flow-report
directory 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=1
would be the second step in the flow andfile:///Users/example/report.html
would be the summary page. In a future PR, we can use something likepreact-router
to manage the URL params instead of doing a full page navigation.