OBPIH-5870 Mandatory comment after reject action (React list page) - Approvals end to end workflow #4332
Conversation
| import apiClient from 'utils/apiClient'; | ||
|
|
||
| export default { | ||
| getStockMovements: config => apiClient.get(STOCK_MOVEMENT_API, config), | ||
| deleteStockMovement: id => apiClient.delete(STOCK_MOVEMENT_DELETE(id)), | ||
| updateStatus: (id, status) => apiClient.post(STOCK_MOVEMENT_UPDATE_STATUS(id), { status }), | ||
| rejectRequest: (id, sender, recipient, comment) => |
There was a problem hiding this comment.
let's do it using ({ id, sender, recipient, comment }) convention, so that you don't have to remember about the order of those arguments which might be annoying.
| className="btn btn-primary align-self-end" | ||
| > | ||
| <Translate id="react.rejectRequestModal.confirmReject.label" defaultMessage="Confirm reject" /> | ||
| </button> |
There was a problem hiding this comment.
I don't know how those buttons look like (it'd be good if you attached a screenshot of the modal), but I'm wondering if it would be possible maybe to use our reusable Button component for that using a proper variant prop?
There was a problem hiding this comment.
That's the same button, so I am changing it.
| }) => { | ||
| const initialValues = { | ||
| sender: { id: currentUser?.id, label: currentUser?.name }, | ||
| recipient: { id: requestor?.id, label: requestor?.name }, |
There was a problem hiding this comment.
I don't mind it, but personally I don't like having all properties in one line, so if it were up to me, I'd do it like:
sender: {
id: currentUser?.id,
label: currentUser?.name,
}etc.
We should set this rule on the ESLint I believe, not to mix the conventions.
| <h4 className="title"> | ||
| <Translate | ||
| id="react.autosaveFeatureModal.title.label" | ||
| defaultMessage={`Please provide a reason for rejecting request: ${id}`} |
There was a problem hiding this comment.
At first I wanted to write that from the user perspective in my opinion it should display a code of a request, not id, but then I realized you do it, so maybe not to cause confusions either rename the prop to identifier (as it is returned from the backend) or even requestCode, but I think we should follow the first approach - so to keep the names the same backend <=> frontend - in this case -> identifier
| <div className="header"> | ||
| <h4 className="title"> | ||
| <Translate | ||
| id="react.autosaveFeatureModal.title.label" |
There was a problem hiding this comment.
react.autosaveFeatureModal.title.label? You sure about that one?
alannadolny
force-pushed
the
OBPIH-5870
branch
from
f392879
to
1e41251
Compare
grails-app/i18n/messages.properties
Outdated
| react.rejectRequestModal.to.label=To | ||
| react.rejectRequestModal.from.label=From | ||
| react.rejectRequestModal.comment.label=Comment | ||
| react.rejectRequestModal.cancel.label=Cancel |
There was a problem hiding this comment.
lets use react.default.button.cancel.label for Cancel and maybe To and From should be defined on react.default.* namespace since these are kind of generic labels rather than rejectRequest specific
There was a problem hiding this comment.
This is a bit of an overkill to create a separate component for Modal buttons, there are only two buttons with no logic, let's move it to RejectRequestModalContent
There was a problem hiding this comment.
just like with the buttons I am not sure if it is necessary to create a whole component to render a h4 with translated text.
But I am 50/50 on this one since it does create a distinct separation of body and header components, so maybe it could stay.
I am interested in what other developers think.
There was a problem hiding this comment.
I created those components for buttons and header because I like when the code is more "structurized". If others think having it in one component is better, I can change it.
There was a problem hiding this comment.
I like the approach of splitting those parts to separate components, because it'd be easier to debug in the future if we knew, that some error/unexpected behavior happens in the header.
There was a problem hiding this comment.
I don't mind the separate header component since it creates something like a semantic separation between header and content components, but my point was, how much benefit do we get from creating a new component for rendering 3 simple tags/components.
We need to be mindful of that and not create an endless nesting of new components.
In my opinion, a new component should be created if:
- It holds a separate logic that can be moved out of the main component (following the principle of separation of concerns)
- It is a reusable component that can be used in other places.
There was a problem hiding this comment.
I wouldn't agree with that, because it would mean that if a component has 3000 lines of code, but doesn't create any separate logic and is not reusable, it should all be done in file.
In general I agree, but readability is also important, hence I don't mind it being done the way Alan did.
| .reject-request-modal-overlay { | ||
| position: fixed; | ||
| top: 0; | ||
| left: 0; | ||
| right: 0; | ||
| bottom: 0; | ||
| background-color: rgba(0, 0, 0, 0.45); | ||
| } |
There was a problem hiding this comment.
This looks like a copy paste of style for configuration-modal-overlay.
The only style that we need is to change the overlay color. react-modal allows us to do so using styles={ { overlay: { background: 'rgba(0, 0, 0, 0.45)'} }} unfortunately we can't set it as default for all Modal component calls so instead I think we can set a global style and overwrite the overlay color globally and maybe remove configuration-modal-overlay class
.ReactModal__Overlay {
background-color: rgba(0, 0, 0, 0.45) !important;
}There was a problem hiding this comment.
this looks like a lot of css that can be replaced with bootstrap classes.
I feel like we need to avoid writing our own CSS if we can since it can be very hard to maintain it in the future and can result in a lot of "dead classes" that are not used anywhere
| .request-modal-textarea { | ||
| height: 150px; | ||
|
|
||
| textarea { | ||
| resize: none; | ||
| height: 150px !important; | ||
| } |
There was a problem hiding this comment.
instead of height: 150px; lets use <textarea /> attribute rows (maybe 7 looks close enough).
and for the resize: none maybe add some props isResizable and logic on Textarea component which adds this styling to the textarea tag.
| .reject-request-modal-content { | ||
| position: absolute; | ||
| top: 20%; | ||
| left: 23%; | ||
| background-color: #ffffff; | ||
| width: 55vw; | ||
| padding: 20px; | ||
| display: flex; | ||
| flex-direction: column; | ||
| } |
There was a problem hiding this comment.
this also looks like a copy of configuration-modal-content class so maybe we should define the default styling for modals somewhere globaly.
| } | ||
|
|
||
| // Classic form | ||
| .classic-form { |
There was a problem hiding this comment.
as much as I would love to use some bootstrap styling or global classes here I understand that it is hard so this can stay.
I was hoping that we can use .panel-body .classic-form classes which are defined on stockMovement.scss but it is written specifically for that page, unfortunately...
If you can do anything to shorten this CSS and maybe use bootstrap classes in some places then it would be great, if not then I won't force you to, unless you want to define some global styles for form input yourself which we will be able to sue in the future.
alannadolny
force-pushed
the
OBPIH-5870
branch
from
058f934
to
4426bfe
Compare
alannadolny
force-pushed
the
OBPIH-5870
branch
from
4426bfe
to
c7221fa
Compare
…Approvals end to end workflow (#4332) * OBPIH-5870 Add translations for new modal * OBPIH-5870 Add components for new modal * OBPIH-5870 Add styling for new modal * OBPIH-5870 Use new modal and allow triggering rejection from it * OBPIH-5870 Add saving rejection reason * OBPIH-5870 Fixes after review * OBPIH-5870 Fixes after review * OBPIH-5870 Add PICKING status to let users fulfill request
No description provided.