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

Remove the inline css in html files and put it to the custom.css file #388

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

Conversation

YoshithaRathnayake
Copy link
Member

@YoshithaRathnayake YoshithaRathnayake commented Feb 2, 2022

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

  • This PR doesn't commit any keys, passwords, tokens, usernames, or other secrets.
  • I have read and understood the development best practices guidelines
  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation

Related PRs

Test environment

Learning

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">
Copy link
Member

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.

Copy link
Member Author

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

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">
Copy link
Member

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">
Copy link
Member

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.

@Piumal1999
Copy link
Member

@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">
Copy link
Member

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
Comment on lines 10 to 11
<div class="row">
<div class="col-md-12 ml-auto mr-auto">
Copy link
Member

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

Copy link
Member Author

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

Copy link
Member Author

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 ?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah

Copy link
Member Author

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

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.

Remove the inline css in html files and put it to the custom.css file
2 participants