-
Notifications
You must be signed in to change notification settings - Fork 50
Blueprints: Add validation warning display to the deeplink flow #2142
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
base: trunk
Are you sure you want to change the base?
Conversation
📊 Performance Test ResultsComparing a1fb2db vs trunk site-editor
site-startup
Results are median values from multiple test runs. Legend: 🟢 Improvement (faster) | 🔴 Regression (slower) | ⚪ No change |
gcsecsey
left a comment
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.
Thanks @ivan-ottinger, this refactor is really solid IMO. I tested opening Blueprints with unsupported features, and also tested uploading Blueprints in the earlier flow. Everything works as described. 👍
I only have minor, non-blocking comments.
| <Heading className="text-center text-[32px] text-gray-900 mb-[59px]" weight={ 500 }> | ||
| { __( 'Start from a Blueprint' ) } | ||
| </Heading> | ||
|
|
||
| <BlueprintWarningNotice | ||
| warnings={ warnings } | ||
| fileName={ blueprintTitle } | ||
| className="w-full max-w-4xl mx-auto mb-4" | ||
| /> | ||
|
|
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.
Let's use the same mb-5 rule on the Heading as in blueprints.tsx to make these pages consistent. We could also increase the bottom margin on the notice itself to a mb-6 to make the layout more consistent.
| PR | Suggestion | Start from Blueprint |
|---|---|---|
![]() |
![]() |
![]() |
| <Heading className="text-center text-[32px] text-gray-900 mb-5" weight={ 500 }> | |
| { __( 'Start from a Blueprint' ) } | |
| </Heading> | |
| <BlueprintWarningNotice | |
| warnings={ warnings } | |
| fileName={ blueprintTitle } | |
| className="w-full max-w-4xl mx-auto mb-6" | |
| /> | |
| description?: string; | ||
| }; | ||
|
|
||
| type BlueprintWarning = { feature: string; reason: string }; |
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.
WDYT about moving this to a shared place, eg. to blueprint-validation.tsx, and reuse it in BlueprintValidationSuccess and for all other occurences in this PR where we specify { feature: string; reason: string; }?
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.
Thanks @ivan-ottinger for adding warnings. I have tested and It looks great. LGTM 👍
We definitely need to do something about the modal in terms of designs. Let's add a task to revisit the designs as soon as we have them. But before that, do you think it makes sense to make some changes to improve it even without the designs? Currently, it looks a bit cramped.



Related issues
Proposed Changes
BlueprintWarningNoticeandBlueprintIssuesModalcomponents into separate fileTesting Instructions
npm install && npm start.blueprint-portfolio-v1.json).Pre-merge Checklist