PLANNER-1515: Add Shift Roster #263
Conversation
There was a problem hiding this comment.
Several questions, issues and suggestions inline.
In addition to that there are a few minor UX issues that would be nice to have fixed:
- Dropdowns should disappear when an option is selected (unless it's a multi-selection dropdown). For example the spot selector on Shift screen, dropdowns in Create Shift dialog.
z-indexof dropdowns in the Create Shift dialog is less than the dialogz-index=> the dropdown is not fully visible.
The lint script is not being run during Maven build. Travis build should fail if npm run lint fails.
employee-rostering-frontend/public/assets/styles/react-big-calendar.css
Outdated
Show resolved
Hide resolved
|
|
||
| componentDidUpdate(prevProps: Props, prevState: State) { | ||
| if (this.props.shift === undefined && prevProps.shift !== undefined) { | ||
| // eslint-disable-next-line react/no-did-update-set-state |
There was a problem hiding this comment.
❔ Question
The reasons behind this warning seems legitimate. Did you try initializing the state from props in the constructor? Is there something that make this approach impossible?
There was a problem hiding this comment.
The reason is transferring changed Prop values into state. Note the constructor does not get recalled if props are changed (clicking a shift set it as the 'selectedShift', which is passed to EditShiftModal (which is already created with an empty/null shift)).
employee-rostering-frontend/src/store/rest/RestServiceClient.test.ts
Outdated
Show resolved
Hide resolved
employee-rostering-frontend/src/domain/indictment/UnavaliableEmployeeViolation.ts
Outdated
Show resolved
Hide resolved
employee-rostering-frontend/src/ui/pages/shift/EditShiftModal.test.tsx
Outdated
Show resolved
Hide resolved
|
@Christopher-Chianelli Please fix "problems" before merging. Questions and suggestions are optional. |
There was a problem hiding this comment.
@yurloc Took your suggestions and fixed all "problems". I also introduced i18n for Alerts.
| i18nKey: string; | ||
| variant: "success" | "danger" | "warning" | "info"; | ||
| params: any; | ||
| components?: React.ReactNode[]; |
There was a problem hiding this comment.
All the above is OK but the components?: React.ReactNode[]; is something I strongly dislike based on what's recommended by Redux documentation. Please take a look at it and tell me what you think. Are there any good reasons for this approach that I might be missing? Is there a hidden issue that might prevent the separation (store: only app state, ui: only React components)?
not that things like that are impossible. It's that they are non-idiomatic, and in the vast majority of use cases, not a good idea.
This is closest to my opinion. Based on what's recommended by the documentation and generally perceived as idiomatic I classify this (putting React nodes) as something we should avoid.
Nice article about Redux store structuring called re-ducks: https://www.freecodecamp.org/news/scaling-your-redux-app-with-ducks-6115955638be/.
There was a problem hiding this comment.
The main reason there is an optional components tab is that some alert messages have structure that need to be preserved. In our case, the "See Stack Trace" button which links to an unique Stack Trace Dialog. The alternatives are to use Functions (another no-no), put a map somewhere that map strings/numbers to functions/components (which seems like bad design), put react components as state (another no-no). The best alternative is probably the map which would be linked to a new component, but in turn it will have a higher maintenance cost.
There was a problem hiding this comment.
Well, you are putting a React component (ServerSideExceptionDialog) into the app state, aren't you? It's inside showServerError() method on line 74. That is a no-no, do you agree?
I think the structure of a stack trace (lines plus caused-by hierarchy) is captured by ServerSideExceptionInfo so why not store just that in the state, without wrapping it into React code?
There was a problem hiding this comment.
The main issue is the "Link", which we need to tell the Alert Component is one way or another is a link instead of text. Currently testing out a solution that uses a map.
|
@yurloc Replaced components with enum + object of basic values, which are used to map to components in |
No description provided.