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

Update number type to support custom currency format #36

Merged
merged 11 commits into from
Feb 28, 2022

Conversation

ghislaineguerin
Copy link
Contributor

@silentninja
Copy link
Contributor

Based on our discussion on Matrix, we should have the UX in such a way that Postgres Money type is discouraged, since it does not bring any advantage over the numeric type + display options, rather has limitations

@kgodey
Copy link
Contributor

kgodey commented Feb 22, 2022

@ghislaineguerin This spec does not address this design problem from mathesar-foundation/mathesar#1063

Users may be confused by two different types to represent money (Money, and Number + display options). We should figure out how to represent this to the user in a way that's easy to understand.

From @pavish in the Matrix discussion about this topic:

If a non-technical user wants to add a money column, how will they know which to use? We provide two separate options, they might add one wrongly or might want to switch from one to another, in which case the display options of number (eg., choosing currency symbol) will be lost.

Their first instinct will be to go to the Money type, and we could have limitations and warnings there, but how do we get the message across that it's a database issue and they'll have to tweak db config, especially considering that they're non-technical.

We would like them to use the display settings of Number, but our UX shows Money more prominently and display options are hidden.

It is a guarantee that the user will always go to the Money type, and how do we redirect it effectively?

@kgodey
Copy link
Contributor

kgodey commented Feb 22, 2022

Potential solutions, not all of these are mutually exclusive.

  • Display money type as "Dollars" (or whatever the locale is set to)
  • Show a warning message when using Money type that we recommend that users use the Number type with currency formatting instead
  • Create a custom "money" type that's actually just a alias for NUMERIC and will need display options set to be useful. Move currency formatting options there.
  • Don't allow users to change column type to the Money type, only show the Money type if there's an existing column that uses it.

Other solutions proposed were:

  • Remove currency formatting from number types entirely (this is not an option since we still need a way for users to store columns with different currencies)
  • Remove support for Money type entirely (this seems unwise since we want to support all default Postgres types)

Copy link
Contributor

@kgodey kgodey left a comment

Choose a reason for hiding this comment

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

Please see my comments on the main thread.

@mathemancer
Copy link
Contributor

Remove support for Money type entirely (this seems unwise since we want to support all default Postgres types)

A middle ground would be to keep support, but make the PostgreSQL Money type an "other" type in the UI. That would discourage users, and give them the expectation that it's not strongly supported, but we wouldn't have to remove any functionality that we currently have.

@kgodey
Copy link
Contributor

kgodey commented Feb 22, 2022

A middle ground would be to keep support, but make the PostgreSQL Money type an "other" type in the UI. That would discourage users, and give them the expectation that it's not strongly supported, but we wouldn't have to remove any functionality that we currently have.

We aren't offering any special display or editing functionality for "other" types. Since we'll already be building in display/editing functionality for working with money, it seems counterintuitive to not offer it for PostgreSQL Money type columns by relegating them to "other".

@kgodey kgodey added the status: waiting A PR that needs follow-up work from its author after review label Feb 22, 2022
@kgodey kgodey added status: review In review and removed status: waiting A PR that needs follow-up work from its author after review labels Feb 24, 2022
Copy link
Contributor

@kgodey kgodey 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 @ghislaineguerin, some minor changes requested.


#### Postgres Money Type Prototype

[Figma Prototype - Postgres Money Type](https://www.figma.com/proto/Uaf1ntcldzK2U41Jhw6vS2/Mathesar-MVP?page-id=7552%3A83433&node-id=7646%3A84762&viewport=241%2C48%2C0.33&scaling=contain)
Copy link
Contributor

Choose a reason for hiding this comment

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

The warning should say:

"The currency setting of this database type cannot be changed."

If the custom Money type is available, it should say:

To use a different currency, please change this type to Number and back to Money.

If the custom Money type is not available, it should say:

Please contact an administrator if you'd like to change the currency setting.

The frontend will know whether the custom Money type is available, so it can use the appropriate message depending on the availability.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This has been updated.


#### Custom Mathesar Money Type Prototype

[Figma Prototype - Custom Mathesar Money Type](https://www.figma.com/proto/Uaf1ntcldzK2U41Jhw6vS2/Mathesar-MVP?page-id=7552%3A83433&node-id=7590%3A84021&viewport=241%2C48%2C0.46&scaling=contain)
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good to me. The only change I'd like is to change the "Symbol Location" options to Beginning and End, I think it's clearer.

@pavish Can you take a look and make sure this works for frontend formatting?

Copy link
Member

Choose a reason for hiding this comment

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

The options look good, it would suffice for frontend formatting.

The number of inputs is many for the display options, it will definitely exceed the window size. We'll need a scrollbar in this pane or change the width for the dropdown. The max-height needs to be fixed.

@ghislaineguerin It would be better to have a prototype with scrollbars.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This has been updated.

Copy link
Contributor

Choose a reason for hiding this comment

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

@ghislaineguerin I don't see a scrollbar in the prototype.


- Select Symbol: Users can set the symbol that will be displayed along with the number.
- Symbol position: Users can select the position for the currency symbol. The options are start or end.
Copy link
Contributor

Choose a reason for hiding this comment

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

These don't seem to match what's in the prototype.

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've fixed this to match the prototype.

@kgodey kgodey removed their assignment Feb 25, 2022
@pavish pavish removed their assignment Feb 25, 2022
Copy link
Contributor

@kgodey kgodey left a comment

Choose a reason for hiding this comment

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

@ghislaineguerin Looks good, the only comment is that I don't see a scrollbar in the prototype (per @pavish's comment).

@ghislaineguerin
Copy link
Contributor Author

@kgodey I have added the scrollbars. I will merge this since that was the only change requested.

@ghislaineguerin ghislaineguerin merged commit 4a42df4 into master Feb 28, 2022
@ghislaineguerin ghislaineguerin deleted the update-money-number-types branch February 28, 2022 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Money type design updates
5 participants