Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BI-674 - Program Definable BrAPI Service URL #75

Merged
merged 10 commits into from
Feb 23, 2021
Merged

Conversation

nickpalladino
Copy link
Member

@nickpalladino nickpalladino commented Feb 5, 2021

Changes

  • Added brapi URL input to program creation form
  • Populated numUsers column in programs table
  • Added brapiUrl to programs table

Notes

  • There are corresponding cards in bi-api and sgn

@nickpalladino nickpalladino marked this pull request as ready for review February 17, 2021 21:19
Copy link
Member

@timparsons timparsons left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@DebWeigand
Copy link

rather than the verbiage next to the checkbox "Specify custom program data storage location", in order to satisfy the first acceptance scenario, would it be beneficial for the text to read "Specify custom program data storage location, otherwise default will be used"?
And, would it be beneficial to spell out the default location?

@nickpalladino
Copy link
Member Author

nickpalladino commented Feb 18, 2021

rather than the verbiage next to the checkbox "Specify custom program data storage location", in order to satisfy the first acceptance scenario, would it be beneficial for the text to read "Specify custom program data storage location, otherwise default will be used"?
And, would it be beneficial to spell out the default location?

I see what you're saying with the description but to me it reads more cleanly to have the checkbox label be more yes/no. After talking with Tim I updated the acceptance criteria to not specify what the default location is because there was implementation implications that didn't seem worth it. Not sure how others feel?

Updated text to say "system default" to match how bi-api is now returning "System Default" rather than the docker URL.

@timparsons

@ctucker3
Copy link
Contributor

ctucker3 commented Feb 22, 2021

Two UI things:

screencapture-localhost-8080-admin-program-management-2021-02-22-11_07_44

Can you add some space between the label and the checkbox? Like an ml-2 or something? It looks cramped.

Also, in the help text, and in the error message for a poorly formatted url can you put an example of what a properly formatted url looks like?

Copy link
Contributor

@ctucker3 ctucker3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment on UI above.

@nickpalladino
Copy link
Member Author

nickpalladino commented Feb 22, 2021

Two UI things:

screencapture-localhost-8080-admin-program-management-2021-02-22-11_07_44

Can you add some space between the label and the checkbox? Like an ml-2 or something? It looks cramped.

I wrapped the input in a label and in browser that added some more space, does it look good on yours?

Also, in the help text, and in the error message for a poorly formatted url can you put an example of what a properly formatted url looks like?

I added an example url to the help text, seems the validation message is automatically generated. I just hardcoded an example for url validation, could be improved in future.

@nickpalladino nickpalladino merged commit f8310fe into develop Feb 23, 2021
@nickpalladino nickpalladino deleted the feature/BI-674 branch February 23, 2021 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants