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

feat(component): add number-unlimited column type to worksheet #1621

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

bc-apostoliuk
Copy link
Contributor

@bc-apostoliuk bc-apostoliuk commented Jan 13, 2025

What?

  • Create new Infinity Icon.
Screenshot 2025-01-14 at 15 11 01
  • Add an action prop for columns whose type is number or text.

Why?

To enable users to add action buttons to these columns.

Screenshots/Screen R

Screen.Recording.2025-01-21.at.15.56.50.mov
Screenshot 2025-01-21 at 17 08 57

Testing/Proof

Unit tests + Locally.

Copy link

changeset-bot bot commented Jan 13, 2025

⚠️ No Changeset found

Latest commit: 608c2fd

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@bc-apostoliuk bc-apostoliuk force-pushed the number-unimited branch 5 times, most recently from a25a7cc to 592480c Compare January 14, 2025 12:51
@bc-apostoliuk bc-apostoliuk marked this pull request as ready for review January 14, 2025 12:51
@bc-apostoliuk bc-apostoliuk requested review from a team as code owners January 14, 2025 12:51
@bc-apostoliuk bc-apostoliuk force-pushed the number-unimited branch 5 times, most recently from be7dc3a to f0b8e07 Compare January 15, 2025 13:03
@jorgemoya
Copy link
Contributor

Seems weird to me that you can make the cell active and the infinity icon is still present, and you have to manually delete. What happens if you try to edit but leave the ∞ there? Does it fail the validation?

I feel like what this is trying to solve is wrong. There should be a checkbox that sets a product as infinite stock. Maybe toggling the checkbox should have some form of logic to change X fields in the row as a response. In this example, setting the checkbox to true would make Stock have no value and disabled.

What do you think?

@bc-jessebayley
Copy link

bc-jessebayley commented Jan 21, 2025

Seems weird to me that you can make the cell active and the infinity icon is still present, and you have to manually delete. What happens if you try to edit but leave the ∞ there? Does it fail the validation?

I feel like what this is trying to solve is wrong. There should be a checkbox that sets a product as infinite stock. Maybe toggling the checkbox should have some form of logic to change X fields in the row as a response. In this example, setting the checkbox to true would make Stock have no value and disabled.

What do you think?

We tested a prototype of the UX as shown in the video up top with some internal ppl (1 support, 1 sales, 1 customer success, 1 PM not on the backorders team).
All of them were able to correctly understand what the infinity symbol would do (it acts as a button to set unlimited). They did not think that in the editing state a value and the icon conflicted, they knew one was the value and the other was a button.
What was a bit confusing/unexpected was editing back from unlimited to a specified limit. In the first pass of the interaction we switch from Unlimited to ∞ because it would be easier for a user to delete. But that ended up confusing ppl because it's not what the cell said, it’s not what they thought they were editing.
So we need an iteration where editing a cell with an "Unlimited" value stays as "Unlimited", and the user has to delete the string (note: users don't actually have to delete one character at a time in the worksheet. They can just select the cell and replace it by hitting 1 or something)
As to your bigger question about this being the wrong solve — having it as a checkbox would introduce new usability issues because the columns in this UI are customisable and rearrangeable. We would never want to show users the backorder limit field without the infinity column seeing as they’re inextricably coupled. We would need to enforce both of these columns being shown or hidden at the same time, we do not have a pattern for that.

@TomA-R TomA-R requested a review from jorgemoya January 21, 2025 01:08
@bc-apostoliuk bc-apostoliuk force-pushed the number-unimited branch 4 times, most recently from 5d0b416 to 608c2fd Compare January 21, 2025 15:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants