OBPIH-5000 Invoice list doesn't store chosen filter params in url #3706
Conversation
…cognized by absolute import eslint mechanism
src/js/hooks/useInvoiceFilters.jsx
Outdated
| createdBy: { name: 'createdBy', accessor: 'id' }, | ||
| }; | ||
|
|
||
| if (Object.keys(values).length > 0) { |
There was a problem hiding this comment.
Object.keys(values).length is always equal or higher than 0, so you can simplify this condition to: Object.keys(values).length. Numbers higher than 0 are truthy, only 0 is falsy.
There was a problem hiding this comment.
@alannadolny You are obviously right, I just didn't catch it when copying this part of code, as it was already implemented - funny thing is that in lines 88 and 91 I caught similar issue and remembered to change
from:
if (!statuses || statuses.length === 0)
to:
if (!statuses || !statuses.length)
kchelstowski
force-pushed
the
OBPIH-5000
branch
from
d65299c
to
c5d0738
Compare
| typeCodes, | ||
| fetchTypeCodes, | ||
| history, | ||
| }) => { | ||
| const [defaultValues, setDefaultValues] = useState({}); |
There was a problem hiding this comment.
I think [defaultValues. setDefaultValues] can also be included in useInvoiceFilters hook
| typeCodes, | ||
| fetchTypeCodes, | ||
| history, |
There was a problem hiding this comment.
I don't really have a problem with passing history as a parameter to this hook but do you think that maybe we could bump up our react-router-dom version to ver 5 where we have useHistory hook available.
If version 5 only adds additional hooks and doesn't change any existing router components or behavior I think we should be fine.
What do you think @awalkowiak ?
There was a problem hiding this comment.
Do you think bumping and router-dom should be included in this ticket if we used to use history using withRouter all around the project?
There was a problem hiding this comment.
I believe as soon as you don’t need anything specific (like I needed with redux hooks), we shouldn’t bump up anything additionally. Imo they should be some small tech debt tickets where we could include some „smoke tests” to see if nothing broke
There was a problem hiding this comment.
This was just my suggestion to avoid passing history as a parameter and use useHistory() instead. I doubt that there would be much refactoring in the project since version 5 does not remove withRouter HOC, but we probably don't really need it and it is not that big of a problem.
There was a problem hiding this comment.
If the update is not "problematic", we could make it (to avoid a bigger refactor in the future and replace everywhere the history props to hook). Unless there are some potential issues, then we can skip it for now.
package-lock.json
Outdated
| @@ -2776,13 +2776,13 @@ | |||
| "alphanum-sort": { | |||
| "version": "1.0.2", | |||
| "resolved": "https://registry.npmjs.org/alphanum-sort/-/alphanum-sort-1.0.2.tgz", | |||
| "integrity": "sha512-0FcBfdcmaumGPQ0qPn7Q5qTgz/ooXgIyp1rf8ik5bGX8mpE2YHjC0P/eyQvxu1GURYQgq9ozf2mteQ5ZD9YiyQ==", | |||
| "integrity": "sha1-l6ERlkmyEa0zaR2fn0hqjsn74KM=", | |||
| typeCodes, | ||
| fetchTypeCodes, | ||
| history, |
There was a problem hiding this comment.
If the update is not "problematic", we could make it (to avoid a bigger refactor in the future and replace everywhere the history props to hook). Unless there are some potential issues, then we can skip it for now.
kchelstowski
force-pushed
the
OBPIH-5000
branch
from
ddfae3b
to
9dbb83b
Compare
No description provided.