-
Notifications
You must be signed in to change notification settings - Fork 2k
Dashboard v2: Add PageHeader
component
#103159
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
Conversation
Jetpack Cloud live (direct link)
Automattic for Agencies live (direct link)
|
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: App Entrypoints (~47 bytes removed 📉 [gzipped])
Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used. Sections (~20 bytes added 📈 [gzipped])
Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to. Async-loaded Components (~34 bytes added 📈 [gzipped])
React components that are loaded lazily, when a certain part of UI is displayed for the first time. Legend What is parsed and gzip size?Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory. Generated by performance advisor bot at iscalypsofastyet.com. |
This is one of these components that I'm not sure should be implemented yet as a reusable component in Would it make sense to hold of making this a reusable component until there's actually a need for it outside the dashboard? |
Also a curiosity that Storybook uses a serif font. Should we apply a |
@jasmussen the spec mentions: I asked Jay today though about whether we would allow both a
That's a tough problem indeed, but do you think that the level detection should be internal of Header component? |
My hesitation here is that I'm not entirely sure what the reusable bits should be, there's too much unknowns. It feels too premature to extract this component. For things like Breadcrumbs or SummaryButton, it's clearer but here I'm not entirely sure how this component translates to other dashboards / plugins ... whether it's tied to navigation or not, whether it's tied to Now, breaking changes are allowed in a8c/components so it's not a strong blocker either. |
I was going to mention the same thing as Riad — I don't think that this component should be in the shared components package for now — its usage feels quite specific to the hosting dashboard, and I think that we should validate more usecases before we can generalize it and eventually extract it to the shared components package.
@jasmussen This is something that I also recently brought up to @mirka . Is the above Based on the answer, we may want to add it just to Storybook, or to some other, more general places in the codebase. Also, when |
That's the de-facto standard, yes, and is the "system fonts" stack we've been using in core as well. However there has been some discussions on whether to use Inter for the A8c side of things. That doesn't mean we can't start with that stack, if you need to, that's certainly what I'd recommend as a starting point. |
I don't know how much it affects the export timeline, but I wanted to point out that this design has been used extensively across divisions in the Forms & Settings atelier, so it will need to be consistently reusable in some way, at some point. It seems beneficial for everyone who needs this component to have access to it asap. So if we don't export now, what questions do we think need answering before we can commit to that? |
Don't consider my comment as a blocker. Especially if there are already usage of this component (and not just hypothetical) outside the hosting dashboard. |
7772769
to
f9dc7df
Compare
@jameskoster with the current code the heading is not well aligned with the actions in dashboard v2, where we use this. Should this alignment be |
@ntsekouras it shouldn't be ![]() We want the decoration / actions to be aligned with the first line of the title like so: ![]() The alignment issue on the host dashboard seems to be caused by two things:
For the second point, I think it would be fine to adjust the metrics in the component to match the host dashboard. I'll work on that now. |
49662c6
to
23c19d0
Compare
This PR modifies the release build for the following Calypso Apps: For info about this notification, see here: PCYsg-OT6-p2
To test WordPress.com changes, run |
64c76c8
to
247e264
Compare
Resolves: DS-166
This PR implements the
PageHeader
component underclient/dashboard
folder.I've also updated the hosting dashboard
v2
to use this component.Notes
backButton
has no designs in the spec/figma, and it will not be a part of the first iteration.v2
where I updated thePageLayout
component. The actual components files are underpage-header
folder.Testing Instructions
yarn storybook:start
v2
all the headers should be fine (in terms of functionality and design wise)Screenshot
Pre-merge Checklist