Skip to content
This repository was archived by the owner on Jan 3, 2023. It is now read-only.

Conversation

@wonmaungthein
Copy link

Hello,

I have just added more details in HTML but haven't started css yet.

index.html Outdated
<header>
<h1 class="heading">CYF</h1>
</header>
<nav-bar>
Copy link
Contributor

Choose a reason for hiding this comment

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

nav-bar is not a valid HTML element .. it should just be nav

index.html Outdated
<article>
<h3>What We Do</h3>
<p>
<strong class="biggertext">CYF</strong> is a fictitious company-lorem ipsum dolor sit amet, whatever. CYF
Copy link
Contributor

Choose a reason for hiding this comment

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

class names, either use dashes as separators which is preferred (so bigger-text) or camelCase. The important thing is to be consistent, and not have it all lowercase as it is hard to read

@@ -0,0 +1,97 @@
.heading {
font-family: 'Apple-Chancery', cursive;
Copy link
Contributor

Choose a reason for hiding this comment

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

this font will probably only work on your machine - you can import Google fonts to ensure that the font is loaded for all users https://fonts.google.com/

Copy link
Author

Choose a reason for hiding this comment

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

Thank you so much for all your help. I am not very sure how to put all google fonts.. Please help me.. Thanks..

main.css Outdated
.heading {
font-family: 'Apple-Chancery', cursive;
font-style: italic;
font-size: 60px;
Copy link
Contributor

Choose a reason for hiding this comment

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

try using "em" and "rem" instead of "px" for units. Read about them if you don't know what they are - they help with doing responsive design and make it scale easier

main.css Outdated
.bordercontainer {
/*border: 3px solid blue;*/
/*display: flex;*/
margin-right: 29%;
Copy link
Contributor

Choose a reason for hiding this comment

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

it is not typical to use percentages for margins and paddings, it works but it is not very standard. You can probably achieve the same using em.

var request = new XMLHttpRequest();
request.open('GET',url)

request.onload = function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

very nice usage of onload (make sure the code is indented properly though)

main.js Outdated
}


var SHnews = document.querySelector('#news-btn');
Copy link
Contributor

Choose a reason for hiding this comment

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

again bad variable names

}

function ShowHide(element) {
if (element.className === "hidden") {
Copy link
Contributor

Choose a reason for hiding this comment

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

nice and simple implementation. well done.

main.js Outdated
var numberInputWV = numberInput.value;

var SubmitFormIsValid = true;
if (nameInputWV.length === 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this check can be if (nameInput.length)

(also pay attention to variable names - what does WV stand for?)

}

if (emailInputWV.length === 0 || emailInputWV.indexOf('@') === -1) {
emailInput.className = 'emailInput inValid';
Copy link
Contributor

Choose a reason for hiding this comment

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

this is correct for the requirement .. Now look into Regular Expressions. See how can you find a RegExp that validates emails. Regular expressions can be quite frightening but at least know how to use them, and understand what they're used for.

@wonmaungthein
Copy link
Author

Hello,

I have modified the page again. Please have a look and give me feedback if you have time. Thank you so much for your help.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants