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

Conversation

Riyuzakii
Copy link

@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

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

@Riyuzakii
Copy link
Author

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

@Riyuzakii
Copy link
Author

Ping @ericholscher .

Copy link
Contributor

@agjohnson agjohnson 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 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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

@agjohnson agjohnson added PR: work in progress Pull request is not ready for full review and removed PR: ready for review labels Mar 21, 2017
@agjohnson agjohnson added PR: work in progress Pull request is not ready for full review and removed PR: ready for review PR: work in progress Pull request is not ready for full review labels May 30, 2017
@RichardLitt
Copy link
Member

@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
Labels
PR: work in progress Pull request is not ready for full review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants