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

Change markdown li break to handle Safari 10.x user stylesheet bug #359

Merged
merged 1 commit into from
Nov 8, 2017

Conversation

slothelle
Copy link

Hey friends 👋 !

I am working on some bugs internally that are related to how long bullets in the body of an issue, PR, and project card are wrapping in Safari. I'm testing locally in Safari 10.1.2 and Chrome 61.0.3163.100 (wow that is a long version number). This bug does not happen with Safari 11.

Based on the testing I've been doing, it looks like word-wrap: break-word; behaves differently for when display is set to list-item. Once I change that display property to literally anything else the bug fixes itself, but we lose the bullet formatting. Changing the li elements to wrap as break-all resolves the issue in Safari, and there are no visible changes in Chrome. I'm happy to screen share with one of y'all and show you what I've been testing over here if you're having trouble reproducing or have a newer version of Safari 😊 !

I'm super new to our CSS and did some poking around in the documentation to see how to go about changing this for dotcom. I am not sure that changing this in Primer is the best approach, but it seems like overriding the Markdown li formatting in our stylesheets for a browser bug is not the right way to go about this either. My thinking regarding changing it in Primer was that because it's a bug with Safari 10.x that is not isolated to GitHub's internal styles, it will impact other folks using Primer. However since this is a bug restricted to an older version of Safari, I can see how we wouldn't want that bloat here.

Let me know what you think! 🙇‍♀️ Happy to pair on an alternative solution also 👏 💯

/cc @primer/ds-core

Copy link
Member

@jonrohan jonrohan left a comment

Choose a reason for hiding this comment

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

I think it’s a great change, 👍 But I wasn’t able to verify the bug in safari v11.x

@slothelle
Copy link
Author

Thanks @jonrohan 😊 ! What's the procedure for merging? I don't have merge access 🌮

@jonrohan
Copy link
Member

jonrohan commented Oct 6, 2017

What's the procedure for merging? I don't have merge access

We'll need to figure out what version we can merge this into, should be going out soon. 9.6 or 10.0. Is there anything hung up on this work?

@slothelle
Copy link
Author

Nope! It's just a bug fix for a minor UI thing that has been open for awhile - not critical, definitely in the "nice to fix" category!

@shawnbot
Copy link
Contributor

shawnbot commented Oct 6, 2017

Let's include it in v10! Thanks, @feministy 🍻

@shawnbot shawnbot added the v10 label Oct 6, 2017
@shawnbot shawnbot changed the base branch from dev to release-10.0.0 October 6, 2017 21:55
@broccolini broccolini changed the base branch from release-10.0.0 to dev November 8, 2017 22:40
@broccolini broccolini changed the base branch from dev to release-10.0.0 November 8, 2017 22:41
@broccolini broccolini merged commit 866e4da into primer:release-10.0.0 Nov 8, 2017
@broccolini broccolini mentioned this pull request Nov 8, 2017
28 tasks
@slothelle slothelle deleted the markdown-list-wrapping branch November 13, 2017 17:06
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.

5 participants