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: add back padding to lists in callouts #126

Merged
merged 4 commits into from
Feb 11, 2021
Merged

Conversation

kellyjosephprice
Copy link
Collaborator

@kellyjosephprice kellyjosephprice commented Feb 11, 2021

Fixes CX-16

🧰 Changes

  • reverts removing left padding from lists in callouts

🧬 QA & Testing

@kellyjosephprice kellyjosephprice temporarily deployed to markdown-pr-126 February 11, 2021 19:23 Inactive
@linear
Copy link

linear bot commented Feb 11, 2021

CX-16 Broken Numbered List in Callouts

Taboola getting frustrated that we haven't fixed a few issues in our callouts when the new markdown engine is enabled. It's been over a year since they reported this.

  1. Ordered lists should be indented in the callout but the callouts don't do any indenting.
  2. They are also noticing that the spacing is off when you add a numbered list to a callout but do not include title.

I've suggested a couple of workarounds that have been completely rejected. Workarounds are not acceptable in this situation.

image (66).png

@kellyjosephprice kellyjosephprice temporarily deployed to markdown-pr-126 February 11, 2021 19:40 Inactive
@kellyjosephprice kellyjosephprice requested review from a team, domharrington and rafegoldberg and removed request for a team February 11, 2021 19:53
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.

Nice; I have one stylistic suggestion below but this is pretty much good. Tangentially, I also noticed some oddities with our demo app:

  • When you try to edit the markdown syntax, each keystroke blurs the editor text area, and makes it a bit of a headache to run a quick syntax test.
  • We're pulling ReadMe's bundle-hub2.css in to the demo apps to better mimic prod. But since that file includes RDMD's current CSS, it can cause some accidental overrides/underrides, and just generally make it kind of hard to test new style changes.

We don't have to address these issues immediately in this PR, just thought I should write 'em down before I forgot!

@kellyjosephprice
Copy link
Collaborator Author

  • When you try to edit the markdown syntax, each keystroke blurs the editor text area, and makes it a bit of a headache to run a quick syntax test.

Yea, I noticed that too. 😬 Should be fixed in the 2nd commit.

@kellyjosephprice kellyjosephprice temporarily deployed to markdown-pr-126 February 11, 2021 21:22 Inactive
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.

👀 Lookin' good; and thanks for fixing up that focus issue!

@kellyjosephprice kellyjosephprice temporarily deployed to markdown-pr-126 February 11, 2021 21:58 Inactive
@kellyjosephprice
Copy link
Collaborator Author

  • We're pulling ReadMe's bundle-hub2.css in to the demo apps to better mimic prod. But since that file includes RDMD's current CSS, it can cause some accidental overrides/underrides, and just generally make it kind of hard to test new style changes.

Ok, not a complete fix, but I imported the local styles into the demo app, so they should override the hubs styling.

@kellyjosephprice kellyjosephprice merged commit 184c238 into next Feb 11, 2021
@kellyjosephprice kellyjosephprice deleted the fix/callout-lists branch February 11, 2021 22:07
@rafegoldberg
Copy link
Contributor

This PR was released!

🚀 Changes included in v6.25.3-next.1

@rafegoldberg
Copy link
Contributor

This PR was released!

🚀 Changes included in v6.26.0

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.

2 participants