Make things pretty #67
Conversation
Theming is set up so that when we decide on fonts, colors, and custom layouts we can update and/or override the default styles using less variables and css.
In an earlier commit I added a site header to App.js I removed it so that the welcome page for new lists can have it's own header. I changed some colors also just to play with it. I'm not married to any style choices I made we can still talk about final design choices.
Instead of making the welcome prompt two parts it's not just one view. The join list field and button are responsive.
I added a drop down with some placeholder data and moved the delete button to inside of it. Also, I took the popup off of the delete button action because it was behaving strangely on mobile.
All the pages have a basic level of styling. For usability I also added a delete item function on the shopping list view. There was a problem where I was losing the token if the addItem screen was the entry to the app. It was because there was no communication with the list in context and we were using that list to check for duplicates. To fix this I added a call to fetchList when addItem loads. Let me know if this breaks anything.
| @@ -24,7 +21,7 @@ | |||
| work correctly both with client-side routing and a non-root public URL. | |||
| Learn how to configure a non-root public URL by running `npm run build`. | |||
| --> | |||
| <title>React App</title> | |||
| <title>Smart Shopping List - TCL3</title> | |||
| "short_name": "React App", | ||
| "name": "Create React App Sample", | ||
| "short_name": "Smart Shopping List", | ||
| "name": "Smart Shopping List - Collab Lab Cohort 3", |
There was a problem hiding this comment.
Love that you took the time to tidy up these details!
| @@ -16,9 +14,17 @@ const ListContextProvider = props => { | |||
| const [name, setName] = useState(''); | |||
| const [shoppingList, setShoppingList] = useState([]); | |||
|
|
|||
| // Should not be exposed in the API of this context | |||
| // This is super simple right now. A better version would have error handling backed into it | |||
| // as well as loading state management. | |||
There was a problem hiding this comment.
Love this comment! It leaves a breadcrumb for a future developer to make the app more resilient. Nice work!
There was a problem hiding this comment.
Oh, I see now this was here before. Still, great comment! 😅
There was a problem hiding this comment.
I think @stevelikesmusic wrote this. I pasted this in from the other pull request. Somehow I lost those functions in the merge and the app was broken.
src/pages/List.js
Outdated
| }; | ||
|
|
||
| switch (nextExpectedPurchase) { | ||
| case 7: |
There was a problem hiding this comment.
This works during testing because each item has the exact values. In practice, items will have values for this variable that are more like 7.342, 2.47, and 14.923 ... unless I'm misunderstanding, any value other than 7, 14, and 30 will result in the item getting the color white.
Instead, you could use a series of if statements, such as the following:
if (nextExpectedPurchase <= 7) {
// set to colorCode.SOON
}
else if (nextExpectedPurchase <= 14) {
// set to colorCode.KIND_OF_SOON
}}
else if (nextExpectedPurchase <= 30) {
// set to colorCode.NOT_SOON
}}
else {
// set to white
}}
There was a problem hiding this comment.
Changed here 110f0bf
| @@ -0,0 +1,3 @@ | |||
| /******************************* | |||
| Site Overrides | |||
There was a problem hiding this comment.
Assuming these are Semantic-UI artifacts? Are they used to override styles of the built-in components? Sure are a lot of them... Is there a way to suppress them if they're not needed?
|
👍 |
| return updateItem(item, purchase); | ||
| }; | ||
|
|
||
| //we use .set to add the time the item was purchased to an already existing document we search for the item.id |
There was a problem hiding this comment.
I think this function an be removed. We dropped it in the last PR in favor of purchaseItem
There was a problem hiding this comment.
I tried to remove it but it was called in purchaseItem
src/pages/AddItem.js
Outdated
|
|
||
| <Container textAlign="center" style={{ paddingBottom: 50 }}> | ||
| <Segment> | ||
| <div> |
There was a problem hiding this comment.
Do we need this div?
src/pages/AddItem.js
Outdated
| <List> | ||
| <List.Item> | ||
| <List.Content> | ||
| <label> |
There was a problem hiding this comment.
Any reason to not use 'Label` from semantic ui?
There was a problem hiding this comment.
The semantic label is under the Form component and I didn't think to convert the forms to Form.
| @@ -1,67 +1,17 @@ | |||
| .container { | |||
There was a problem hiding this comment.
❤️ deleting lots of code 😂
| event.preventDefault(); | ||
| saveToken(joinToken); | ||
|
|
||
| return joinToken && history.push('/list'); |
There was a problem hiding this comment.
Nice use of history to redirect 👏
| "build": "react-scripts build", | ||
| "test": "react-scripts test", | ||
| "eject": "react-scripts eject", | ||
| "start": "craco start", |
There was a problem hiding this comment.
Was this to allow overriding some of semantic ui's less styling?
There was a problem hiding this comment.
Yes, the Semantic UI docs recommended craco so that the app doesn't eject when the less for the custom theme compiles. I'm not too familiar with that but that's how I understood it.
| @@ -1,4 +1,3 @@ | |||
| //normalized function | |||
I cleaned up a an unneeded div tag, removed some blank lines, and moved the updateItem function closer to purchaseItem in listContext.js.
This PR addresses all of the styling issues #37, #38, #39, #40. We are going to make the app look pretty.
We agreed on using semantic UI for a component library. In the first commit of this branch Semantic has been imported and the NavTab menu was changed to use the Semantic menu.