Skip to content
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

fix(Header, Cards, Hero): fix spacings logic #1298

Merged
merged 1 commit into from
Nov 26, 2024

Conversation

marcoskolodny
Copy link
Contributor

@marcoskolodny marcoskolodny commented Nov 26, 2024

With these fixes, the only screenshot failing in webapp is this one:

image

But this is expected (and it was incorrect before). This is a DataCard with headline + extra. According to card specs, there is no space between pre-defined content and extra slot:

image

@@ -424,10 +424,12 @@ const CardContent = ({
</div>
)}
{headline && (
// assuming that the headline will always be followed by one of: pretitle, title, subtitle, description
Copy link
Contributor Author

@marcoskolodny marcoskolodny Nov 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All cards were assuming this before, except of AdvancedDataCard. In #1292, I've done the same for the AdvancedDataCard, and it broke one screenshot test in webapp (they use headline and extra prop to render everything else).

I've removed all these unnecesary assumptions from everywhere

Copy link

Size stats

master this branch diff
Total JS 12.2 MB 12.2 MB -796 B
JS without icons 2.02 MB 2.02 MB -796 B
Lib overhead 69 kB 69 kB 0 B
Lib overhead (gzip) 16.9 kB 16.9 kB 0 B

@@ -107,7 +109,7 @@ export const Header = ({
)}
{headlineContent}
{pretitleContent && (
<div style={{paddingBottom: pretitle || description ? 8 : 0, order: -1}}>
<div style={{paddingBottom: title || description ? 8 : 0, order: -1}}>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was a typo :(

Copy link

Accessibility report
✔️ No issues found

ℹ️ You can run this locally by executing yarn audit-accessibility.

Copy link

Deploy preview for mistica-web ready!

✅ Preview
https://mistica-igpbtgsfr-flows-projects-65bb050e.vercel.app

Built with commit 5906817.
This pull request is being automatically deployed with vercel-action

@marcoskolodny marcoskolodny requested a review from atabel November 26, 2024 12:32
@marcoskolodny marcoskolodny added this pull request to the merge queue Nov 26, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 26, 2024
@marcoskolodny marcoskolodny added this pull request to the merge queue Nov 26, 2024
Merged via the queue into master with commit 327ac5f Nov 26, 2024
11 checks passed
@marcoskolodny marcoskolodny deleted the fix-headings-spacing branch November 26, 2024 13:08
tuentisre pushed a commit that referenced this pull request Nov 26, 2024
## [16.6.1](v16.6.0...v16.6.1) (2024-11-26)

### Bug Fixes

* **Header, Cards, Hero:** fix spacings logic ([#1298](#1298)) ([327ac5f](327ac5f))
@tuentisre
Copy link
Collaborator

🎉 This PR is included in version 16.6.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants