Skip to content

Conversation

@kellyjosephprice
Copy link
Collaborator

@kellyjosephprice kellyjosephprice commented May 29, 2024

PR App Fixes RM-9863, RM-9864

🧰 Changes

Fixes empty header callout styling.

🧬 QA & Testing

@kellyjosephprice kellyjosephprice changed the base branch from next to beta May 29, 2024 21:05
@kellyjosephprice kellyjosephprice marked this pull request as ready for review May 29, 2024 21:26
Copy link
Contributor

@rafegoldberg rafegoldberg left a comment

Choose a reason for hiding this comment

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

Tried this locally and it's working great save for one edge case I noticed.

{empty || React.Children.toArray(children)[0]}
</h3>
{empty ? children : React.Children.toArray(children).slice(1)}
{React.Children.toArray(children).slice(1)}
Copy link
Contributor

@rafegoldberg rafegoldberg May 30, 2024

Choose a reason for hiding this comment

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

Why do we not need this ternary logic anymore? Is it possibly related to this discrepancy?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It was a little simpler to leave the title/heading as an empty child. But we could just as easier refactor the code not to.

border-color: var(--border);
color: var(--text);
padding: $l-offset;
min-height: 3 * $l-offset;
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm, interesting that we need to set a minimum height at all here… (And also kinda strange to see this left offset var being used to set the height!)

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably not a huge deal, but it does suggest that there was some underlying change to the callout markup? It'd be nice to know what that is/why it broke the layout!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is/was actually a pre-existing condition? Perhaps it was a regression I introduced, but it is happening in core @readme/markdown.

Copy link
Contributor

@rafegoldberg rafegoldberg May 30, 2024

Choose a reason for hiding this comment

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

Ahhh, I see, I see… Unlikely to be something you introduced though! I probably just never tested my original CSS against an entirely empty callout since it’s (technically) not a case that we support.

Copy link
Contributor

@rafegoldberg rafegoldberg May 30, 2024

Choose a reason for hiding this comment

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

Interesting—even GitHub doesn't handle empty quotes particularly gracefully… Check it out:

Honestly, thinking about it a little harder now, there shouldn't really ever be a scenario where you'd have an empty callout. Maybe we can just remove this line entirely?

Suggested change
min-height: 3 * $l-offset;

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

A+

Copy link
Contributor

@rafegoldberg rafegoldberg left a comment

Choose a reason for hiding this comment

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

I'm good if we want to merge this in as-is though, so long as we add a ticket for this to our Fast Follows milestone. (I also linked RM-9864, since it looks like that's also solved herein!)

@kellyjosephprice kellyjosephprice merged commit 463b870 into beta May 30, 2024
@kellyjosephprice kellyjosephprice deleted the fix/callout-styling branch May 30, 2024 18:07
rafegoldberg pushed a commit that referenced this pull request May 30, 2024
## Version 6.75.0-beta.45

### 🛠 Fixes & Updates

* callout styling ([#891](#891)) ([463b870](463b870))

<!--SKIP CI-->
@rafegoldberg
Copy link
Contributor

This PR was released!

🚀 Changes included in v6.75.0-beta.45

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants