Skip to content

Pablo's Page - #14

Open
pamta wants to merge 13 commits into
AlexHenkel:masterfrom
pamta:master
Open

Pablo's Page#14
pamta wants to merge 13 commits into
AlexHenkel:masterfrom
pamta:master

Conversation

@pamta

@pamta pamta commented May 17, 2018

Copy link
Copy Markdown

Hello Alex, this is the first version of my page.

@AlexHenkel AlexHenkel left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Good job. Follow the requested changes


<!-- Bootstrap -->
<link rel="stylesheet" href="Bootstrap/css/bootstrap.min.css" />
<link rel="stylesheet" href="Bootstrap/css/bootstrap.css" />

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

As you can see, you are only using 2 bootstrap files here, so you should remove all the unused files that you have in Bootstrap/css and also you should check the js folder as well.

It also happens that Bootstrap/css/bootstrap.min.css and Bootstrap/css/bootstrap.css is the same file, the difference is that the first one is compressed (aka. minified, that's where the min comes from). The difference is just that the min file has lower size and it's used usually in production where you don't need a clean code (just take a lot at that file and you won't understand anything). So just leave this file bootstrap.min.css and remove the other one.

In web development there's a usual convention where all the folders are in lower case, so you would rename the folder to bootstrap. But also, since you will delete all the unused bootstrap files, you will only end up with bootstrap.min.css and bootstrap.min.js, so you should store those files in your css and js folders without creating an extra folder for just bootstrap. And you should do that as well if you ever use an external library as well, because you would only use 1 or 2 files, which will be the minified versions of the library.

<script src="Bootstrap/js/bootstrap.min.js"></script>
</head
<body>

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Add a bootstrap container here, it will help to give a fixed layout working with in all devices
https://getbootstrap.com/docs/4.0/layout/overview/#containers

<div>
<h1>About</h1>

<div class = "row">

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Don't leave extra spaces in attributes. That should be written like: <div class="row">
Obviously you can use spaces inside the quotes, but outside the quotes you should not leave spaces. If you ever need a two-word attribute for an html tag, you should use a tag and always lower case (Ex. <div data-content="This is content"></div>)

<!--div location-->
<div>
<h2>Our Locations</h2>
<ul>

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

<div>
<h2>Upcoming Classes</h2>

<table>

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Take a look at bootstrap's tables: https://getbootstrap.com/docs/4.1/content/tables/

<div>
<h2>Sign Up!</h2>

<label>

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Wrap all the form inputs into a <form> tag and also check bootstrap's forms: https://getbootstrap.com/docs/4.1/components/forms/

</div> </br>

<!--Social div-->
<div>

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Maybe use a <footer> tag here. It won't make any difference in the rendering, but it makes your code more understandable.

border: 1px solid black;
}

.Gender-fs{

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Don't use upper case ever when naming classes or id's

}


img{

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

@pamta

pamta commented Jun 19, 2018

Copy link
Copy Markdown
Author

Hello Alex, the first verions of the page that works with a js script, is ready.

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.

2 participants