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

4789 item variant UI pt2 #5227

Merged
merged 10 commits into from
Oct 25, 2024
Merged

4789 item variant UI pt2 #5227

merged 10 commits into from
Oct 25, 2024

Conversation

lache-melvin
Copy link
Contributor

Fixes #4789

πŸ‘©πŸ»β€πŸ’» What does this PR do?

Moves add/edit of item variants to modal:
Screenshot 2024-10-25 at 10 30 27β€―AM

πŸ’Œ Any notes for the reviewer?

πŸ§ͺ Testing

  • No backend wired up, but check add/edit open the modal as expected, and send the right payload to the backend

πŸ“ƒ Documentation

  • Part of an epic: documentation will be completed for the feature as a whole
  • No documentation required: no user facing changes or a bug fix which isn't a change in behaviour
  • These areas should be updated or checked:
    1.
    2.

@github-actions github-actions bot added this to the V2.4.0 milestone Oct 24, 2024
@github-actions github-actions bot added enhancement New feature or request Team Piwakawaka James, LachΓ©, Carl needs design Needs PO/UI/UX designer input feature: GAPS labels Oct 24, 2024
},
{
key: 'name',
Cell: update ? TextInputCell : TooltipTextCell,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

thoughts on logic here to be able to reuse the table component?

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems good πŸ‘πŸΌ

fullWidth
/>
}
Input={<BasicTextInput value={variant.name} disabled fullWidth />}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could have reused the edit form from the modal here, but am under the impression that item variants are likely to end up with a lot of fields, and probably only want a subset in the tab view πŸ‘€

Copy link
Contributor

Choose a reason for hiding this comment

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

Really? I would have thought we'd want to see the same fields on the tab view as in the modal. How many are we talking?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we're talking long term, but that's what Adam has said 😁 I think I'll stick with it anyway - nice not to do things like query all manufacturers for the dropdown, only to display read only data

Copy link

github-actions bot commented Oct 25, 2024

Bundle size difference

Comparing this PR to main

Old size New size Diff
4.87 MB 4.87 MB 0 B (0.00%)

@CarlosNZ CarlosNZ self-assigned this Oct 25, 2024
Copy link
Contributor

@CarlosNZ CarlosNZ 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, allowing for the TO-DOs and the other considerations we discussed IRL.

Maybe just chuck in the empty array when no variants defined, and add the "No data" component as discussed?

},
{
key: 'name',
Cell: update ? TextInputCell : TooltipTextCell,
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems good πŸ‘πŸΌ

fullWidth
/>
}
Input={<BasicTextInput value={variant.name} disabled fullWidth />}
Copy link
Contributor

Choose a reason for hiding this comment

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

Really? I would have thought we'd want to see the same fields on the tab view as in the modal. How many are we talking?

@lache-melvin lache-melvin merged commit f895b95 into develop Oct 25, 2024
2 of 3 checks passed
@lache-melvin lache-melvin deleted the 4789-item-variant-ui-pt2 branch October 25, 2024 03:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request feature: GAPS needs design Needs PO/UI/UX designer input Team Piwakawaka James, LachΓ©, Carl
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GAPS: Implement item variants: Item UI changes
2 participants