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

BOSE landing page #2594

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

BOSE landing page #2594

wants to merge 63 commits into from

Conversation

zaura333
Copy link

@zaura333
Copy link
Author

For now it's only styled for mobile (320px)

@zaura333
Copy link
Author

Now I have to work on menu, styles for buttons and inputs, then I'll make it responsive

Copy link

@JoyCoffeeAddict JoyCoffeeAddict left a comment

Choose a reason for hiding this comment

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

You page should work one every mobile device, so you can't just go: "it works on 320px mobile"
Using hard-coded values for things like navbar is a bad idea:
.header { grid-template-columns: repeat(2,130px);}
it will make your life harder later

@zaura333 zaura333 requested a review from mvjl000 October 23, 2023 10:34
Copy link

@JoyCoffeeAddict JoyCoffeeAddict left a comment

Choose a reason for hiding this comment

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

  1. Add a favicon
  2. Center the text vertically
    image
  3. I would make the phone number clickable here"
    image
  4. Checklist points 4, 8, 15, 18

Other than that, I think the page looks good. Tag us here or ping us on the slack if you don't understand any of the issues.

@zaura333
Copy link
Author

I made favicon out of sound waves image as the logo isn't really fit for that use, can change back if needed

Also I changed articles in recommend and categories sections into links. Texts scale alongside the images because I thought it might make sense in the context of this page. If that's not right for the task, I'll restructure that as well! 😀

Copy link

@JoyCoffeeAddict JoyCoffeeAddict left a comment

Choose a reason for hiding this comment

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

  1. There is some horizontal scrolling on my screen
    image

  2. Text scaling is not a bad idea,but I think that only one image should scale on hover
    image

Some small adjustments and you're done! I think that the page is great.

Copy link

@mvjl000 mvjl000 left a comment

Choose a reason for hiding this comment

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

This looks great! 🚀

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