the-collab-lab / tcl-28-smart-shopping-list

1 stars 3 forks source link

add features to the app #65

Closed hulyak closed 3 years ago

hulyak commented 3 years ago

Description

Let me know what you think.

Related Issue

closes #51

Acceptance Criteria

Type of Changes

Type
:bug: Bug fix
:sparkles: New feature
:hammer: Refactoring
:100: Add tests
:link: Update dependencies
:scroll: Docs

Updates

Before

Screen Shot 2021-09-08 at 10 05 39 PM

After

Screen Shot 2021-09-08 at 10 06 16 PM Screen Shot 2021-09-08 at 9 55 17 PM Screen Shot 2021-09-08 at 9 55 24 PM Screen Shot 2021-09-08 at 9 24 34 PM Screen Shot 2021-09-08 at 9 54 59 PM

Testing Steps / QA Criteria

github-actions[bot] commented 3 years ago

Visit the preview URL for this PR (updated for commit c616fe5):

https://tcl-28-shopping-list--pr65-hulya-custom-dropdow-om5ixcz7.web.app

(expires Sat, 18 Sep 2021 21:26:40 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

isaabutaa commented 3 years ago

Hi @hulyak! Nice job taking initiative and adding some additional features and styles to the app. I only have a couple critiques, but nothing major.

First, I really like the "Would you like to invite your friends to your shopping list?" idea. I know TCL-27 did that, and it's a nice touch. If the user hovers over that text, the cursor changes to a pointer, so they know they can click on it, but I wonder if maybe we can add a small border, or slightly different background color with a drop shadow to make it more obvious that it's clickable?

Second, on the Items List page, I noticed that you added the list items that act as a reference so the user knows which color means what — I think that's a good idea. At first, I thought that it was the Add Item form because it looks very similar, but then I realized it was different. I don't know if I have any actionable recommendations for that, but it's just a thought I had.

Also, I realized that the font we're importing, Asap, isn't the font that's on the app. I think it's just sans-serif. If that's something we want to change, my recommendation would be to copy + paste the Google fonts link in the header in public/index.html, and then put the font in the styles page, or import the font in the styles page:

Link in header in public/index.html:

<header>
    <link rel="preconnect" href="https://fonts.googleapis.com">
    <link rel="preconnect" href="https://fonts.gstatic.com" crossorigin>
    <link href="https://fonts.googleapis.com/css2?family=Asap&display=swap" rel="stylesheet">
</header>

Import in root css file:

@import url('https://fonts.googleapis.com/css2?family=Asap&display=swap');

And then to actually use the font, the simplest solution might be to do font-family: 'Asap', sans-serif; in the body selector of the root css file.

Those are just some of my thoughts, but again, way to take initiative. I know we ran out of time last week, and I think it's totally valid to want to add more to the app. 👍

hulyak commented 3 years ago

Thanks, @isaabutaa for reviewing the PR and writing a very good review. I checked the font, it's actually ASAP. Screen Shot 2021-09-09 at 3 17 08 PM

I have added a border for the token prompt and changed some styles. You can check out the new version. Thanks!