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

Minor CSS Fixes #254

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Minor CSS Fixes #254

wants to merge 5 commits into from

Conversation

KartikSoneji
Copy link

Minor CSS Fixes:

  • Fix clipped heading anchors because of overflow: hidden
    Closes CSS issue in headings #150
  • Remove *zoom hack for IE7
  • Use opacity instead of display to hide heading anchors
  • Remove unused h1 a and h2 a styles
  • Fix header anchor aligment

@itamarhaber
Copy link
Member

Thank you @KartikSoneji!

This appears to do the job :)

image

Minor nitpicks:

  • It looks like the asterisk is lower than it should be (visually)
  • Does it make sense to replace the asterisk with a link icon (e.g. fa-link?)

Also, IIUC, the *zoom: 1 is the IE7 hack so that's good. I'm curious, however, as to how you verified that the h1 and h3 stuff is not needed?

@KartikSoneji
Copy link
Author

It looks like the asterisk is lower than it should be (visually)

The original website had the asterisk even lower, I centred it vertically (by eye). I assumed the original designer used them as a stylized bullet (•Title), not as an asterisk (*Title).

image

Does it make sense to replace the asterisk with a link icon (e.g. fa-link?)

I think both are equally valid, it depends on the visual design.
Although it would be better to use a pseudo element like ::before instead of an actual * in the markup, since pseudo elements cannot be selected or copied, and the asterisk is styling that should not be included in the content.
This also has the benefit that if the style changes in the future, it only has to be updated in one place.

I'm curious, however, as to how you verified that the h1 and h3 stuff is not needed?

Oops. I just checked the output of grep -r "<h1" and grep -r "<h3" without noticing the templates were not HTML.
The styles will need to be updated, the current version (on redis.io) is broken for h3.

image

Would you prefer I revert the commit or rebase and force-push?

@itamarhaber
Copy link
Member

I think both are equally valid

I trust your eye - please feel free to change/re-style/keep as is.

... or force push

Force push is fine :)

@KartikSoneji
Copy link
Author

Is there any particular reason for cramming each rule into a single line?
More importantly, is there a good reason to preserve the formatting?

Also, both views/grid.scss and views/normalize.scss don't seem to be used, nor does the project download or use the sass compiler. Is there any point in keeping the files in the repo?

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.

CSS issue in headings
2 participants