Skip to content

buble theme: strong font-weight increased to 600 #913

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

Closed
wants to merge 1 commit into from
Closed

buble theme: strong font-weight increased to 600 #913

wants to merge 1 commit into from

Conversation

xmedeko
Copy link
Contributor

@xmedeko xmedeko commented Aug 22, 2019

Summary

What kind of change does this PR introduce? (check at least one)

  • Bugfix
  • Feature
  • Code style update
  • Refactor
  • Docs
  • Build-related changes
  • Other, please describe:

If changing the UI of default theme, please provide the before/after screenshot:

Does this PR introduce a breaking change? (check one)

  • Yes
  • No

If yes, please describe the impact and migration path for existing applications:

The PR fulfills these requirements:

  • When resolving a specific issue, it's referenced in the PR's title (e.g. fix #xxx[,#xxx], where "xxx" is the issue number)

You have tested in the following browsers: (Providing a detailed version will be better.)

  • Chrome
  • Firefox
  • Safari
  • Edge
  • IE

@anikethsaha
Copy link
Member

anikethsaha commented Nov 21, 2019

Is there any specific issue or link which might support this change?
I need some reason to back these changes.

I can see you checked bugfix box, need an issue link or any related links to back this

Thanks

@xmedeko xmedeko changed the title strong font-weight increased to 600 buble theme: strong font-weight increased to 600 Nov 21, 2019
@xmedeko
Copy link
Contributor Author

xmedeko commented Nov 21, 2019

@anikethsaha The bug is, that the bold font is not recognizable from the normal fint in the buble theme. I do not know what kind of link should I provide. Should I make one more bug issue about that?

@anikethsaha
Copy link
Member

anikethsaha commented Nov 21, 2019

@xmedeko actually the optimal flow is to have an issue and then discussing there we can decide whether its a bug or not and PR is required or not.

Leave it for this one. No need to create an issue. We can discuss this here just for this PR.

Also, can you provide some screenshots comparing before-after .

PS

font-weight for strong in default is bold which is 700.
Any specific reason you selected 600 ?

Comment on lines 34 to 40
.markdown-section h1, .markdown-section h2, .markdown-section h3, .markdown-section h4, .markdown-section strong
color #333
font-weight 400
font-weight 600
Copy link
Member

Choose a reason for hiding this comment

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

The default of these is bold in the browser. I am pretty sure this is an intended code.

@xmedeko
Copy link
Contributor Author

xmedeko commented Nov 21, 2019

If you make markdown bold **sometext** , i.e. <strong> in HTML, then the text is almost no different to other, normal text in the buble theme. So it's unusable. I have taken the code from other themes: doplhin, vue, ... see the similar code there.

It's possible the fix is not well for this theme. It should affect headings also, but the buble theme has just a few lines bellow:

.markdown-section a
  ...
  font-weight 400

And it overwrites the fix font-weight 600 for some (!) headings. So this value should be changed too. Or maybe let the headings as they are: 400 and separate .markdown-section strong? When about is I make a link with a strong text?

I think the theme has a few more troubles, but I like it. I am not so strong in CSS. Since the docsify has been kind of unmaintained with lot of open issues, I have decided to post this hot fix PR instead of opening one more issue.

If you can make a better fix of buble theme, then go ahead (also not wrong spelling of "buble").

@anikethsaha
Copy link
Member

#913 (comment)

Actually thats where we are struggling a bit. The former maintainers I guess are not active and not maintaining so we are struggling to get the indentions of these few codes. Like this one. I am not sure why at first place these where 400 as these are not the strong styling (as you said too)
I am actually +1 for this fix but not getting the motive of keeping it 400 at first place.

I think we will go with this PR and if required we will revert this if it is suggested.

I am down to 600 or 700. can you please investigate if anything is breaking or looking weird with having 700 as that's the default by browsers

@xmedeko
Copy link
Contributor Author

xmedeko commented Nov 21, 2019

Since the doplhin and vue are using 600, I vote for 600. I thing the reason is to be kind of lighter feeling. Also, because both these styles have same value for .markdown-section a, I would change it there too. I may amend the commit or do it by yourself.
Thanks.

@anikethsaha
Copy link
Member

Since the doplhin and vue are using 600, I vote for 600

Ok let's go with 600.

Copy link
Member

@anikethsaha anikethsaha left a comment

Choose a reason for hiding this comment

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

Thanks for PR and detailed explanation
👍

@xmedeko
Copy link
Contributor Author

xmedeko commented Nov 21, 2019

Hold on, I've been playing with that a bit and seems to me the 400 suites the headings and links better in this theme. Maybe separate strong style could be better.

@anikethsaha
Copy link
Member

Hold on, I've been playing with that a bit and seems to me the 400 suites the headings and links better in this theme. Maybe separate strong style could be better.

separate the strong from this group 👍

@xmedeko
Copy link
Contributor Author

xmedeko commented Nov 21, 2019

Ok, will do it tomorrow.

@anikethsaha
Copy link
Member

ohh shoot!!
I forgot, can you please submit PR against the master branch

@xmedeko
Copy link
Contributor Author

xmedeko commented Nov 22, 2019

@anikethsaha I've separated strong and set it to 600. I've removed the a { font-weight: 400}, because it has overridden the headings h1, ... font-weight below.

(Links has blue color in the buble theme, so they do not need the slight font-weight increase. I also do not know how to keep font-weight and do not override headings, because headings are links to. I aware of CSS !important, but I think this solution is better for this situation.)

I've rebased to master. You should set the default branch to master from develop.

@anikethsaha
Copy link
Member

You should set the default branch to master from develop.

We are discussing that. Actually develop is for v5 but there were quite a good number of bugs that needed to be fixed before v5 so we are working on final v4 release.
So we need to have that discussing then we can change the default branch.
So fast merging I would recommend submitting a new PR against master

@xmedeko
Copy link
Contributor Author

xmedeko commented Nov 22, 2019

Ok, closing this PR and have made #952

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

Successfully merging this pull request may close these issues.

2 participants