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.

Sign up for GitHub

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

Jump to bottom

Program #27

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Program #27

wants to merge 4 commits into from

Conversation

Osei-b4
Copy link
Collaborator

@Osei-b4 Osei-b4 commented May 6, 2023 •
edited
Loading

Description

A clear and concise description of what the Pull Request is about.

General Checks

  • the branch is up to date with main/master
  • the code works when pulled and run locally
  • All CI checks pass
  • all conflicts are resolved (if any)
  • PR has a descriptive title
  • PR has appropriate labels and milestones for easy identification
  • PR it is assigned to the owner
  • reviewers are assigned
  • the PR contributes only one focused change
  • It is in the appropriate column in the project board (if necessary)
  • has short and clear description
  • is linked to an issue (if it is related)
  • feedback is addressed (if any and if it is appropriate feedback.)

Markdown

  • the markdown source is formatted
  • spelling and grammar is correct in all text
  • The markdown looks correct when you preview the file
  • all links and images work

HTML

  • pages besides /index.html are stored in /pages
  • the code is well-formatted
  • the HTML is valid
  • there are no inline styles (example: style='color: red;')
  • there are no <style> tags with CSS, all styles are hrefs
  • there is no inline JavaScript (example: onclick='doSomething()')
  • there are no <script> tags with JS, all JS is in an separate file
  • ids are used for JavaScript only, not for CSS
  • semantic tags are used
  • spelling and grammar is correct in all site content

CSS

  • the code is well-formatted
  • passes all the linting checks

@Osei-b4 Osei-b4 added the program label May 6, 2023
@Osei-b4 Osei-b4 self-assigned this May 6, 2023
@Osei-b4 Osei-b4 linked an issue May 10, 2023 that may be closed by this pull request
program #26
Open
@bermarte
Copy link
Member

Hi, you should add a reviewer.

ThibautHumblet
ThibautHumblet requested changes May 21, 2023
View reviewed changes
Copy link

@ThibautHumblet ThibautHumblet left a comment

Choose a reason for hiding this comment

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

Hi @Osei-b4 ,

First of all, nicely done. I've added a small suggestion to make your code more maintainable. You can change your code however you want, but the least different classes, the better. Especially if it isn't nessesary for the functionality of the project. Apart from this, very good job! Looking forward to your future pull requests.
If you have any more questions, feel free to contact me 😄

Cheers!

pages/program.html
Comment on lines +22 to +43
<nav class="header-nav">
<a href="url" class="nav-link1">
<span>THE PROGRAM</span>
</a>

<a href="url" class="nav-link2">
<span>VOLUNTEER</span>
</a>

<a href="url" class="nav-link3">
<span>DIGITALRNTS</span>
</a>

<a href="url" class="nav-link4">
<span>ABOUT</span>
</a>
<a href="" class="nav-link5">
<span>FAQ</span>
</a>
<a href="url" class="nav-link6">
<span>SUPPORT US</span>
</a>
Copy link

@ThibautHumblet ThibautHumblet May 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Is there any reason why you're rewriting the same style over and over again? You can reuse the nav-link class for each <a>-tag.

Suggested change
<nav class="header-nav">
<a href="url" class="nav-link1">
<span>THE PROGRAM</span>
</a>
<a href="url" class="nav-link2">
<span>VOLUNTEER</span>
</a>
<a href="url" class="nav-link3">
<span>DIGITALRNTS</span>
</a>
<a href="url" class="nav-link4">
<span>ABOUT</span>
</a>
<a href="" class="nav-link5">
<span>FAQ</span>
</a>
<a href="url" class="nav-link6">
<span>SUPPORT US</span>
</a>
<nav class="header-nav">
<a href="url" class="nav-link">
<span>THE PROGRAM</span>
</a>
<a href="url" class="nav-link">
<span>VOLUNTEER</span>
</a>
<a href="url" class="nav-link">
<span>DIGITALRNTS</span>
</a>
<a href="url" class="nav-link">
<span>ABOUT</span>
</a>
<a href="" class="nav-link">
<span>FAQ</span>
</a>
<a href="url" class="nav-link">
<span>SUPPORT US</span>
</a>

Now you can remove all the styles. This makes your code way more manageable in the long run. I've added a suggestion in the .css file.

styles/program.css
Comment on lines +75 to +151
.nav-link1 {
display: inline-flex;
-webkit-box-align: center;
align-items: center;
height: 100%;
color: rgb(127, 127, 127);
font-size: 12px;
line-height: 160%;
text-transform: uppercase;
padding: 10px 20px;
font-family: "Work Sans", sans-serif;
}

.nav-link2 {
display: inline-flex;
-webkit-box-align: center;
align-items: center;
height: 100%;
color: rgb(127, 127, 127);
font-size: 12px;
line-height: 160%;
text-transform: uppercase;
padding: 10px 20px;
font-family: "work sans", sans-serif;
}

.nav-link3 {
display: inline-flex;
-webkit-box-align: center;
align-items: center;
height: 100%;
color: rgb(127, 127, 127);
font-size: 12px;
line-height: 160%;
text-transform: uppercase;
padding: 10px 20px;
font-family: "Work Sans", sans-serif;
}

.nav-link4 {
display: inline-flex;
-webkit-box-align: center;
align-items: center;
height: 100%;
color: rgb(127, 127, 127);
font-size: 12px;
line-height: 160%;
text-transform: uppercase;
padding: 10px 20px;
font-family: "Work Sans", sans-serif;
}

.nav-link5 {
display: inline-flex;
-webkit-box-align: center;
align-items: center;
height: 100%;
color: rgb(127, 127, 127);
font-size: 10px;
line-height: 160%;
text-transform: uppercase;
padding: 10px 20px;
font-family: "Work Sans", sans-serif;
}

.nav-link6 {
display: inline-flex;
-webkit-box-align: center;
align-items: center;
height: 100%;
color: rgb(127, 127, 127);
font-size: 10px;
line-height: 100%;
text-transform: uppercase;
padding: 10px 20px;
font-family: "Work Sans", sans-serif;
}
Copy link

Choose a reason for hiding this comment

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

As I mentioned before in the .html file, this code can be drastically reduced by maintaining just a single style and reusing this for every link. This reduces your code by almost 75 (repeated) lines.

Suggested change
.nav-link1 {
display: inline-flex;
-webkit-box-align: center;
align-items: center;
height: 100%;
color: rgb(127, 127, 127);
font-size: 12px;
line-height: 160%;
text-transform: uppercase;
padding: 10px 20px;
font-family: "Work Sans", sans-serif;
}
.nav-link2 {
display: inline-flex;
-webkit-box-align: center;
align-items: center;
height: 100%;
color: rgb(127, 127, 127);
font-size: 12px;
line-height: 160%;
text-transform: uppercase;
padding: 10px 20px;
font-family: "work sans", sans-serif;
}
.nav-link3 {
display: inline-flex;
-webkit-box-align: center;
align-items: center;
height: 100%;
color: rgb(127, 127, 127);
font-size: 12px;
line-height: 160%;
text-transform: uppercase;
padding: 10px 20px;
font-family: "Work Sans", sans-serif;
}
.nav-link4 {
display: inline-flex;
-webkit-box-align: center;
align-items: center;
height: 100%;
color: rgb(127, 127, 127);
font-size: 12px;
line-height: 160%;
text-transform: uppercase;
padding: 10px 20px;
font-family: "Work Sans", sans-serif;
}
.nav-link5 {
display: inline-flex;
-webkit-box-align: center;
align-items: center;
height: 100%;
color: rgb(127, 127, 127);
font-size: 10px;
line-height: 160%;
text-transform: uppercase;
padding: 10px 20px;
font-family: "Work Sans", sans-serif;
}
.nav-link6 {
display: inline-flex;
-webkit-box-align: center;
align-items: center;
height: 100%;
color: rgb(127, 127, 127);
font-size: 10px;
line-height: 100%;
text-transform: uppercase;
padding: 10px 20px;
font-family: "Work Sans", sans-serif;
}
.nav-link {
display: inline-flex;
-webkit-box-align: center;
align-items: center;
height: 100%;
color: rgb(127, 127, 127);
font-size: 12px;
line-height: 160%;
text-transform: uppercase;
padding: 10px 20px;
font-family: "Work Sans", sans-serif;
}

Another way to tackle this can be by reusing the .css code but with different classes. You can achieve this by doing the following:

Suggested change
.nav-link1 {
display: inline-flex;
-webkit-box-align: center;
align-items: center;
height: 100%;
color: rgb(127, 127, 127);
font-size: 12px;
line-height: 160%;
text-transform: uppercase;
padding: 10px 20px;
font-family: "Work Sans", sans-serif;
}
.nav-link2 {
display: inline-flex;
-webkit-box-align: center;
align-items: center;
height: 100%;
color: rgb(127, 127, 127);
font-size: 12px;
line-height: 160%;
text-transform: uppercase;
padding: 10px 20px;
font-family: "work sans", sans-serif;
}
.nav-link3 {
display: inline-flex;
-webkit-box-align: center;
align-items: center;
height: 100%;
color: rgb(127, 127, 127);
font-size: 12px;
line-height: 160%;
text-transform: uppercase;
padding: 10px 20px;
font-family: "Work Sans", sans-serif;
}
.nav-link4 {
display: inline-flex;
-webkit-box-align: center;
align-items: center;
height: 100%;
color: rgb(127, 127, 127);
font-size: 12px;
line-height: 160%;
text-transform: uppercase;
padding: 10px 20px;
font-family: "Work Sans", sans-serif;
}
.nav-link5 {
display: inline-flex;
-webkit-box-align: center;
align-items: center;
height: 100%;
color: rgb(127, 127, 127);
font-size: 10px;
line-height: 160%;
text-transform: uppercase;
padding: 10px 20px;
font-family: "Work Sans", sans-serif;
}
.nav-link6 {
display: inline-flex;
-webkit-box-align: center;
align-items: center;
height: 100%;
color: rgb(127, 127, 127);
font-size: 10px;
line-height: 100%;
text-transform: uppercase;
padding: 10px 20px;
font-family: "Work Sans", sans-serif;
}
.nav-link1, .nav-link2, .nav-link3, .nav-link4, .nav-link5, .nav-link6 {
display: inline-flex;
-webkit-box-align: center;
align-items: center;
height: 100%;
color: rgb(127, 127, 127);
font-size: 12px;
line-height: 160%;
text-transform: uppercase;
padding: 10px 20px;
font-family: "Work Sans", sans-serif;
}

However, I would suggest using the first example. Because there isn't any benefit for using a different class for every link 😄

pages/program.html
<main class="mani-container">
<section class="introduction-section">
<div class="introduction-container">
<div class="txet-container">
Copy link

Choose a reason for hiding this comment

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

Small typo, your .css class "text-container" won't be linked 😇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ThibautHumblet ThibautHumblet ThibautHumblet requested changes

Requested changes must be addressed to merge this pull request.

Assignees

@Osei-b4 Osei-b4

Labels
program
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

program
3 participants
@Osei-b4 @bermarte @ThibautHumblet