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

Picko – A business site #369

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

Conversation

HeleneWestrin
Copy link

I set out to create a hero section with a video background with a sign up incorporated within. As a bonus I also added a section underneath with ads and a filtering option on categories.

Additionally I added a hamburger menu that can be opened and closed. The JavaScript I have added was written with the help of ChatGPT. I provided it with my markup and what I wanted to achieve.

If I had more time I would have looked into creating a custom select dropdown so that I could style the options as well.

I've also explored using CSS Nesting since it newly became a baseline feature of CSS.

See it live: https://picko.netlify.app/

Copy link
Contributor

@HIPPIEKICK HIPPIEKICK left a comment

Choose a reason for hiding this comment

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

Wow, you've done an amazing job with this project! 🎉 Your code is well-organized, and you've clearly put a lot of thought into making it responsive, functional and look nice. It's great to see you experimenting with JavaScript as well 🥳

HTML

  • Well structured HTML overall, with semantic elements.
  • One small thing to note: You’ve used a non-standard quotation mark in one of your <link> tags:
<link rel=”mask-icon” href=”./assets/favicon.svg” color=”#000000">

The correct quotation marks should be the standard double quotes ("). It's a minor issue but worth fixing to avoid any potential browser inconsistencies.

CSS

  • Nice usage of CSS variables ⭐
  • You've used classes effectively to apply styles and kept the rules DRY (Don't Repeat Yourself).

Keep up the great work! I'm looking forward to seeing how your coding evolves with more projects. 🚀

Comment on lines +21 to +22
width="120"
height="35"
Copy link
Contributor

Choose a reason for hiding this comment

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

Keep the styling to the CSS file 👀

@HeleneWestrin
Copy link
Author

Thank you so much @HIPPIEKICK 🙏
I really appreciate your feeback!

Regarding the width and height in the HTML on the images I actually added that after doing a Lighthouse inspection of my finished project. It suggested to always add width and height to images to avoid layout shifts 😅

<!-- dont forget to add a css file and link it here! -->
<title>Picko — When you like to move it, move it</title>
<link rel="preconnect" href="https://fonts.googleapis.com" />
<link rel="preconnect" href="https://fonts.gstatic.com" crossorigin />

Choose a reason for hiding this comment

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

Your code is clear and clean and it was extremely hard to find anything to pick on but I had a try :'). Firstly, the inclusion of fonts using tags is a really good idea and with the use of "preconnect" it should improve performance by reducing latency for external resources - very nice!

loop
preload="auto"
poster="./assets/video-poster.jpg"
>
Copy link

@jacquelinekellyhunt jacquelinekellyhunt Sep 8, 2024

Choose a reason for hiding this comment

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

The use of a video background adds a cool touch to the hero section. You have included in video element attributes (muted, plays inline, autoplay, loop) for smooth functionality - but I read that maybe a controls attribute might be a good fallback option in case autoplay fails.

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 Kelly, I'll look into that 🙏

justify-content: center;
width: 1.5rem;
height: 100%;
background: url('./assets/icon-error.svg') center no-repeat;

Choose a reason for hiding this comment

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

Read that the label:has() CSS selector is a good use for validation, but this may have limited browser support (e.g., only supported in Safari currently). Read it in this stack overflow post: https://stackoverflow.com/questions/35993727/not-selector-not-behaving-the-same-between-safari-and-chrome-firefox

Copy link
Author

Choose a reason for hiding this comment

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

I looked into it and using this website you can check how well supported a selector is as of today:
https://caniuse.com/?search=has

The Stack overflow thread is 8 years old so a lot has happened since then 🙂


const header = document.querySelector(".page-header");
window.addEventListener("scroll", () => {
header.classList.toggle("is-scrolled", window.scrollY > 100);

Choose a reason for hiding this comment

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

Just want to say you're an inspiration for learning so quickly everything and trying new things - this whole (js) part is still an alien world to me but interesting to read!
Hopefully, I provided at least something new for you to look at but overall everything seems so good! <3

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 Kelly! 🤗 I assure you that I also find JavaScript challenging 😅 I got some help from ChatGPT for this project.

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.

3 participants