admin-template-add

Clone Tools
  • last updated a few seconds ago
Constraints
Constraints: committers
 
Constraints: files
Constraints: dates
Feel free to close the review.

Feel free to close the review.

Already implemented as instructed here on commit OLMIS-5959: Enable facility type button and set facility type input required (https://github.com/OpenLMIS/openlmis-requisition-ui/commit/1446f6a3209...

Already implemented as instructed here on commit OLMIS-5959: Enable facility type button and set facility type input required (https://github.com/OpenLMIS/openlmis-requisition-ui/commit/1446f6a3209ba7245e04f5adc85d4d4258917b52)

I don't think we can do that as we still have one unresolved comment and the changes requested are not yet implemented.

I don't think we can do that as we still have one unresolved comment and the changes requested are not yet implemented.

Hello, should i close this review?

Hello, should i close this review?

Hello Nikodem Graczewski, no problem

Hello Nikodem Graczewski, no problem

Ian marking this as unresolved so we can track the progress on it.

Ian marking this as unresolved so we can track the progress on it.

OLMIS-5959: Enable facility type button and set facility type input required

Okay, will do so

Okay, will do so

Yes, please revert those changes and mimic how the "Child Nodes" section of the https://test.openlmis.org/#!/administration/supervisoryNodes/supervisoryNodes/fb38bd1c-beeb-4527-8345-900900329c10?pa...

Yes, please revert those changes and mimic how the "Child Nodes" section of the https://test.openlmis.org/#!/administration/supervisoryNodes/supervisoryNodes/fb38bd1c-beeb-4527-8345-900900329c10?page=0&size=10&nodesPage=0&nodesSize=10 look like:
1. Add a header saying "Facility Types" (without the asterisk).
2. Make Facility Type (with asterisk) label, select and add button in line and aligned to the right (making it a form within a div with the openlmis-table-container class should do the trick, if not, please refer to the styleguide).
3. Make the select editable at all time and required.
4. The "You need to select program first" should be placed under the "Facility Types" header.
5. User should be able to submit the Requisition Template form without selection Facility Type.
6. User should not be able to submit the Facility Type form without selecting it.
7. Facility Type select should clear when the program is changed.

Should I revert back to required?

Should I revert back to required?

I don't see why we should move adding a simple separator to another ticket and introduce confusion for facility type select here. It should not go in this shape into the nex release.

I don't see why we should move adding a simple separator to another ticket and introduce confusion for facility type select here. It should not go in this shape into the nex release.

We could, but it should be done in the scope of a different ticket and also fix other places that might have the same issue. Would you be willing to create the ticket?

We could, but it should be done in the scope of a different ticket and also fix other places that might have the same issue. Would you be willing to create the ticket?

I think maybe we should separate the facility type select from other selects to indicate it is different form.

I think maybe we should separate the facility type select from other selects to indicate it is different form.

In my opinion, the Facility Type select was working as intended. The asterisk should be there as the "Facility Type" select is a part of the form for adding entries to the table, not the whole "Req...

In my opinion, the Facility Type select was working as intended. The asterisk should be there as the "Facility Type" select is a part of the form for adding entries to the table, not the whole "Requisition Template" form. The issue described in OLMIS-5959 is still valid (and was most likely introduced by the solution to OLMIS-5848 i.e. removal of the "required" property of the "Facility Type" select).

Also, Mateusz Kwiatkowski is correct on the disabling part. We're not doing it as it might confuse the user.

I think that those 2 tickets have the wrong requirements, also as I said we should not disable buttons. Nikodem Graczewski how this should be solved?

I think that those 2 tickets have the wrong requirements, also as I said we should not disable buttons. Nikodem Graczewski how this should be solved?

Agree with you but there was another ticket which says facility type is not mandatory (https://openlmis.atlassian.net/browse/OLMIS-5848)

Agree with you but there was another ticket which says facility type is not mandatory (https://openlmis.atlassian.net/browse/OLMIS-5848)

Actually I liked the previous version more, I think that in this project we agreed that we should not disable any buttons, instead, we are making some fields required and then we can simply block s...

Actually I liked the previous version more, I think that in this project we agreed that we should not disable any buttons, instead, we are making some fields required and then we can simply block submitting the form and show error message (I guess it should be done automatically).

Noted, thank you

Noted, thank you

Added on this commit (OLMIS-5959:Disable facility type add button until facility type is selected), also changed the logic instead of making facility type required i just disable add button because...

Added on this commit (OLMIS-5959:Disable facility type add button until facility type is selected), also changed the logic instead of making facility type required i just disable add button because facility type is not supposed to be mandatory

OLMIS-5959: Disable facility type add button untill facility type is selected

  1. … 1 more file in changeset.
Generally we are adding at least 2 people to reviews, I'll add Nikodem Graczewski since he is responsible for UI mostly

Generally we are adding at least 2 people to reviews, I'll add Nikodem Graczewski since he is responsible for UI mostly

Please add changelog entry

Please add changelog entry

OLMIS-5959: Make facility type input required on add template form
OLMIS-5959: Make facility type input required on add template form
OLMIS-5959: Make facility type input required on add template form

OLMIS-5848: Fixed a bug with facility type marked as required when creating a requisition templat
OLMIS-5848: Fixed a bug with facility type marked as required when creating a requisition templat