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

changed <strong> from html to css #2699

Closed
wants to merge 1 commit into from
Closed

Conversation

@Riyuzakii
Copy link

@Riyuzakii Riyuzakii commented Mar 6, 2017

@agjohnson i just learned html /css last night so tell if i can make some corrections.

@agjohnson i just learned html /css last night so tell if i can make some corrections.
@Riyuzakii
Copy link
Author

@Riyuzakii Riyuzakii commented Mar 6, 2017

@ericholscher is this alright or some thing else needs to be done.

@Riyuzakii
Copy link
Author

@Riyuzakii Riyuzakii commented Mar 9, 2017

@ericholscher Isn't this what you wanted me to do with the files from #2503 (review).

@Riyuzakii
Copy link
Author

@Riyuzakii Riyuzakii commented Mar 19, 2017

Ping @ericholscher .

Copy link
Contributor

@agjohnson agjohnson left a comment

Thanks for the patch! I raised a couple of issue with this PR, and I realize you're just learning some of this -- feel free to raise questions if my feedback isn't clear

<th><p class ="strong">Day (UTC)</p></th>
<th><p class ="strong">Views</p></th>
<th><p class ="strong">Clicks</p></th>
<th><p class ="strong">CTR</p></th>
Copy link
Contributor

@agjohnson agjohnson Mar 21, 2017

My feedback to the original approach was that this didn't need to use <strong> tags on each cell. Instead of applying <strong> or <p class="strong"> to each <th>, we should instead be applying a heavier weight to the table tr th css selector.

p.strong {
font-weight: bold;
}
</style>
Copy link
Contributor

@agjohnson agjohnson Mar 21, 2017

We don't use inline styles, you'll find our css at media/css/core.css

@RichardLitt
Copy link
Member

@RichardLitt RichardLitt commented Dec 21, 2017

@Riyuzakii Thanks for the work, but this has been open a bit long! You're welcome to keep contributing in the future. :)

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

Successfully merging this pull request may close these issues.

None yet

5 participants