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

Taha's got issues #35

Open
Taha-Hassan-Git opened this issue Jun 21, 2023 · 1 comment
Open

Taha's got issues #35

Taha-Hassan-Git opened this issue Jun 21, 2023 · 1 comment

Comments

@Taha-Hassan-Git
Copy link

Love the Atoll joke.

Design is super cool, you've hit all the acceptance criteria from what I can see.
I love the animate on scroll banner component. Very nice touch.
The price filtering is also really slick.
I can see you've used the context API, which is cool.
Local storage is working and persisting data!

Some small issues, mostly to do with the UI.

  • When there are two rows of islands on the home page, scrolling to the bottom of the page causes the screen to shake and scroll back up
  • Islands look very small on mobile
  • More of a question- does the Banner event listener need to be in a useEffect?

V minor, but I enjoy doing these:
Shouldn't this...

  const allRegions = [...new Set(getAllRegions().map(region => region.region))];

  const allPrices = [...new Set(getAllPrices().map(price => price.price))];

...be this?

  const allRegions = [...new Set(getAllRegions().map(island => island.region))];

  const allPrices = [...new Set(getAllPrices().map(island => island.price))];

🤷🏽

@camelPhonso
Copy link
Contributor

@Taha-Hassan-Git, they should be that, yes!! I can't believe I didn't spot that.. ugly 😞

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

No branches or pull requests

2 participants