-
Notifications
You must be signed in to change notification settings - Fork 31
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
Remove the inline css in html files and put it to the custom.css file #388
base: master
Are you sure you want to change the base?
Remove the inline css in html files and put it to the custom.css file #388
Conversation
index.html
Outdated
@@ -74,7 +74,7 @@ <h4 class="card-title">Nam aliquam bibendum ullamcorper. Praesent tempor</h4> | |||
<div class="section pt-0 "> | |||
<h2 class="section-name"> Quick Access </h2> | |||
<div class="d-flex justify-content-center"> | |||
<div class="row" style="width: 71rem;"> | |||
<div class="row quick-access-cards"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job @YoshithaRathnayake
Could you please try to add this change using existing bootstrap CSS?
You can refer to this: https://getbootstrap.com/docs/4.0/layout/grid/#offsetting-columns
Btw, I think it will be easier for you to complete #358 and #375 together with the same PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I thought that puting in bootstrap is good
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the custom width and move the row into a container. It will set the width automatically.
index.html
Outdated
@@ -11,7 +11,7 @@ | |||
<div class="col-md-12 ml-auto mr-auto"> | |||
<div class="title text-left"> | |||
<h1>St. Anthony's College</h1> | |||
<p style="font-family: 'Roboto'; font-size: 20px; font-weight: 400;"> | |||
<p class="since-btn"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's just use an h4
instead of custom css.
index.html
Outdated
@@ -74,7 +74,7 @@ <h4 class="card-title">Nam aliquam bibendum ullamcorper. Praesent tempor</h4> | |||
<div class="section pt-0 "> | |||
<h2 class="section-name"> Quick Access </h2> | |||
<div class="d-flex justify-content-center"> | |||
<div class="row" style="width: 71rem;"> | |||
<div class="row quick-access-cards"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the custom width and move the row into a container. It will set the width automatically.
@YoshithaRathnayake, Since #387 is already merged, please do the changes requested in #387 (comment), #387 (comment) within this pull request itself. |
index.html
Outdated
@@ -70,7 +74,7 @@ <h4 class="card-title">Nam aliquam bibendum ullamcorper. Praesent tempor</h4> | |||
<div class="section pt-0"> | |||
<h2 class="section-name"> Quick Access </h2> | |||
<div class="d-flex justify-content-center"> | |||
<div class="row" style="width: 71rem;"> | |||
<div class="row container"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's move the container class to line 76. Also, we can remove d-flex justify-content-center
classes because there's no any changes from those.
<h2 class="section-name"> Quick Access </h2>
<div class="container">
<div class="row">
index.html
Outdated
<div class="row"> | ||
<div class="col-md-12 ml-auto mr-auto"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any specific reason for using a row and a col here? I think it works the same even without the row and column
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any specific reason for using a row and a col here? I think it works the same even without the row and column
Yeah I checked nothing happening
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So shall I remove that @Piumal1999 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah
Ok @Piumal1999 I removed that
Purpose
The purpose of this PR is to fix #375
Goals
Removing the inline css in html files and put it to the custom.css file
Approach
I removed the inline css in html files and put it to the custom.css file
Screenshots
No screenshots
Preview Link
https://pr-388-sack-site-acf.surge.sh
Checklist
Related PRs
Test environment
Learning