-
Notifications
You must be signed in to change notification settings - Fork 177
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
[instrument_builder] ADDED: field warnings for maximum length in Instrument Builder module #8112
[instrument_builder] ADDED: field warnings for maximum length in Instrument Builder module #8112
Conversation
Great stuff, Satvik! It would be great to have an idea of how the warnings will appear to the end user - could you kindly post a screenshot in this thread, showing the error message in context? Thanks, that would be really helpful in speeding this through the review process. |
@christinerogers Thanks! Here are the screenshots |
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.
Hi @satvik-tha-god thanks for this PR!
I've suggested edits for the error messages to let the user know directly what they should do (shorten it). Thanks Satvik for checking over the numbers I included.
@racostas - over to you to check if this satisfies the issue / all cases etc.
Hi @satvik-tha-god, |
Co-authored-by: christinerogers <[email protected]>
Co-authored-by: christinerogers <[email protected]>
Co-authored-by: christinerogers <[email protected]>
Co-authored-by: christinerogers <[email protected]>
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.
Hi @satvik-tha-god,
Two minor changes:
- The label of the question don't have to be restricted to 64 characters, just the field_name.
- You will find a test Plan on the module folder. Please let update it accordingly with this new feature to be tested.
failing a the test suit, nevertheless I don't see direct relation avec this PR itself. What do you think @driusan ? |
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.
LGTM. 👍
@racostas it looks like this is in your court to re-review, unless i'm reading it wrong? |
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.
Thank you @satvik-tha-god for those fixes.
I think the only thing missing is to update the modules/instrument_builder/test/TestPlan.md
accordingly. Could you please take a look into it when you get a chance ?
I'm cc @christinerogers as she might want to take a look as well since she identified the original issue.
I think we can merge this and then send another PR to fix the test plan. |
…t Builder
Brief summary of changes
_status
fields:Testing instructions (if applicable)
Link(s) to related issue(s)
_status
fields) #7582