-
Notifications
You must be signed in to change notification settings - Fork 41
Feat/fe challenge #21
base: main
Are you sure you want to change the base?
Conversation
Adds in the BE a filter to get the given statuses
devServer: { | ||
hot: true, | ||
inline: false | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added this for when we want to run the app only in the frontend to see live reloads
@@ -14,6 +15,7 @@ | |||
"react": "^17.0.2", | |||
"react-dom": "^17.0.2", | |||
"react-scripts": "4.0.3", | |||
"swr": "^1.3.0", | |||
"typescript": "^4.1.2", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added these libs to reuse components from the design system as well as benefit from the abstractions of SWR such as handling side effects on render that SWR brings.
|
||
if (!isLoading && policies && policies.length !== 0 && policiesState.allPolicies.length === 0) { | ||
dispatchPolicies({type: ActionsTypes.LoadPolicies, payload: policies}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here I am aware it is not a production-ready solution, however, it is a first step to seeing the feature working. The following steps would be things like: Moving the fetching of policies, and dispatching of actions to a custom hook or some other solution, so that we can keep App as the entry point, free of specific logic
value: currentProvider, | ||
}) | ||
} | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are different unrefined ideas I have here at the moment to iterate over.
- Create something like a SelectFilter, that would encapsulate the logic that would get an array of options and fill them as data. This component could potentially avoid the need to create separately the options object of each of the specifics defined above and might make it more scalable.
- Rudimentary debounce needs benchmarking
let paginationButtons = []; | ||
|
||
for (let index = 0; index < maxPages ; index++) { | ||
paginationButtons.push(<button key={Math.floor(Math.random() * 1000)} onClick={(e) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the current use case the key calculation should work, however, if we want it to be future proof we would need to invest a bit more of the software scientist's brain to see this solution will be enough
{paginationButtons} | ||
</nav> | ||
</div> | ||
</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: none of the apps so far is a11y compliant, the solutions added here are not either. For the button for example I'd add an aria-label: page 1 of bla
return newState; | ||
case ActionsTypes.FilterByProvider: | ||
newState.currentPolicies = filter(state.allPolicies, (policy:Policy) => policy.provider === action.payload) | ||
return newState; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yet all these case seem repetitive, adding an abstraction to reduce the number of cases, felt for the current use case, a premature optimization fo the code. At this point I figured having "3 glasses of water" would do
What?
How to test?
What's not included?