-
Notifications
You must be signed in to change notification settings - Fork 9
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
feat: technique creation #628
Conversation
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.
Kindly go through my comments and reply if have any concerns. Overall it is a good work and a complete package.
apps/frontend/src/components/technique/CreateUpdateTechnique.tsx
Outdated
Show resolved
Hide resolved
apps/frontend/src/components/technique/AssignInstrumentsToTechniques.tsx
Outdated
Show resolved
Hide resolved
@yoganandaness These are some really good comments, thanks Yoganandan. We have addressed most of these now but have left a couple of things with comments to clarify. |
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.
Thanks for the response. Found some more improvements, when i went through the PR. Could you please take a look.
Kindly let me know in case of any concerns.
<AssignInstrumentsToTechniques | ||
assignInstrumentsToTechniques={assignInstrumentsToTechniques} | ||
close={(): void => setOpenTechniqueAssignment(false)} | ||
instrumentIds={(selectedTechnique?.instruments || []).map( |
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.
Based on the type definition of TechniqueFragment, the instruments field is always going to be present and i dont think there is a real need for the validation here.
apps/frontend/src/components/technique/AssignInstrumentsToTechniques.tsx
Outdated
Show resolved
Hide resolved
apps/frontend/src/components/technique/AssignInstrumentsToTechniques.tsx
Outdated
Show resolved
Hide resolved
apps/frontend/src/components/technique/AssignInstrumentsToTechniques.tsx
Outdated
Show resolved
Hide resolved
apps/frontend/src/components/technique/AssignInstrumentsToTechniques.tsx
Outdated
Show resolved
Hide resolved
apps/frontend/src/components/technique/AssignInstrumentsToTechniques.tsx
Outdated
Show resolved
Hide resolved
@yoganandaness Deepak has now added a video above 🙂 |
event.loggedInUserId, | ||
event.type, | ||
json, | ||
obj[0].techniqueId, |
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.
If this id not a string already we must need to convert it to a string.
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.
Since obj is from JSON.parse(), its of type any. It's getting converted to string type when getting passed to this method call. I think, explicit conversion is not required here right?
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.
This looks good to me just , that one comment.
Overall, it looks great. Just a small comment. Thanks. |
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.
Looks good to me.
Description
This PR creates "Techniques" which are accessible via a new tab in the User Officer role. You can create/edit/delete them and add a name, short code, description and attach instruments (experimental areas) to them.
Note that this is not hidden behind a feature flag so it is currently visible to all facilities.
Motivation and Context
This is the first part of creating an STFC Xpress system in the software. You can see our plan so far and more discussion in UserOfficeProject/issue-tracker#1032 (comment)
How Has This Been Tested
Fixes
Closes UserOfficeProject/issue-tracker#1095
Changes
Depends on
The schema validation has already been merged via UserOfficeProject/user-office-lib#194
Tests included/Docs Updated?
GMT20240627-101014_Recording.cutfile.20240627101603583_1920x1200.mp4