footer refactor #2
Conversation
There was a problem hiding this comment.
tldr: Really appreciate your proposal. If you make some changes I will be glad to merge it.
There are some issues in the change, I believe you choose a defensive style to introduce your changes, but it makes it impossible to merge the change as is.
I really appreciate the React considerations, some techniques I never used and it is good to see how others do stuff. I will learn about those techniques thanks to you.
The project does not have a linter today, I think some things in this PR (4 spaces) should have been caught by the linter. If you want to add a linter in a separate PR, that would be super valuable.
web/src/App.js
Outdated
| @@ -6,6 +6,7 @@ import { rootReducer } from './redux'; | |||
| import Footer from "./Footer"; | |||
| import Service from "./Service"; | |||
| import FilterBar from "./FilterBar"; | |||
| import { Footer2 } from "./Footer2"; | |||
There was a problem hiding this comment.
Call it Footer. We either merge your PR or not. This defensive approach just makes the change messy.
web/src/App.js
Outdated
| @@ -18,57 +19,58 @@ function App() { | |||
|
|
|||
| return ( | |||
| <> | |||
| <APIBackend capacitorClient={capacitorClient} store={store}/> | |||
There was a problem hiding this comment.
This seems like a format change. The things inside <> are moved one to the right.
Please remove this so the PR clearly shows what the material changes inside.
You can file a formatting PR separately.
web/src/Footer2.jsx
Outdated
| return ( | ||
| <div aria-labelledby="slide-over-title" role="dialog" aria-modal="true" className={`fixed inset-x-0 bottom-0 bg-neutral-200 border-t border-neutral-300 ${expanded ? 'h-2/5' : 'h-16'}`}> | ||
| <div className={`flex justify-between w-full ${expanded ? '' : 'h-full'}`}> | ||
| <div className='h-auto w-full cursor-pointer' |
There was a problem hiding this comment.
You use 4 spaces, the codebase uses 2 I think. Please adjust the PR.
(Also if you stick to the Footer component it is easier to follow the changes.)
web/src/Sidebar.jsx
Outdated
|
|
||
|
|
||
|
|
||
| export const SideBar = memo(function SideBar(props) { |
There was a problem hiding this comment.
Good idea to move the Sidebar into its own file. But please remove the original Sidebar as well in this change.
web/src/Sidebar.jsx
Outdated
| @@ -0,0 +1,39 @@ | |||
| import React, { memo } from 'react'; | |||
|
|
|||
web/src/Sidebar.jsx
Outdated
| </ul> | ||
| </nav> | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Add a new line at the end of each file.
|
This is top notch! Thank you for your contribution! |
No description provided.