Text Data Type

[ghislaineguerin]

The specs related to issue #234 are ready for review. Please take a look at the attached document and follow the review guidelines.

Document Link

https://wiki.mathesar.org/en/design/specs/data-types-text

@mathemancer @kgodey @pavish, your feedback is required.
The deadline for feedback is set to a week from now, the first review round will be completed by Friday 23rd. Please plan your review so that at least one round of changes can be made and reviewed if needed.

[kgodey]

Thanks for your work on this, @ghislaineguerin. Some feedback:

• The "Text" and "Long Text" distinction seems arbitrary and I'm not sure why we have two different types.
  • Mapping DB data types to Mathesar data types #269 lists the types that I have specced out for Mathesar currently. There's only one "text".
    • That mapping isn't set in stone, I'm happy to modify it if needed, but I'm not sure why we need a separate type for "long text".
  • The character limit of 255 for "Text" also seems arbitrary. PostgreSQL has no such limit, the user can set the max length to any positive integer.
• We need to be able to show advanced users the underlying database type (which could be TEXT, VARCHAR or CHAR). I don't see any place for this in the current design.
• I see there's a third dropdown in grouping options with "A-Z". We don't currently have plans to support different orders of grouping in the backend, they'll just be able to group by first letter A-Z. If we want to support more advanced sorting-within-grouping, we should do that post-MVP unless you think it's really necessary for the first iteration.
• I think it would be good to have the grouping and sorting available from the column header as well. If I'm working with a column, It would be nice to take actions on that column from the column itself, rather than having to switch context to "Table Actions".
• I think we should have a single case representation for the data type. Currently the visible type is in uppercase, but when you change types, it switches to sentence case. Ideally, we would show the "Mathesar type" in a different case from the DB type, to distinguish them.
• Under the "Group By" options, the way that the text is positioned is harder to parse than it could be. It would be nice to have it read "Group <release_name> by " rather than have "GROUP BY" above multiple dropdowns. The same applies to sorting.

[ghislaineguerin]

@kgodey Thanks for your comments. I have updated based on our discussion earlier today

[kgodey]

@ghislaineguerin Thanks, looks great! Feedback:

• Could you update the spec document to match the prototype? It still mentions text and long text, which is confusing.
• The "Advanced Options" menu currently replaces the main column menu, which is confusing without context. It would be nice if it could be next to the main column menu or otherwise has more context about which column it's related to.
• There should be a save button for the "Advanced Options" menu, since it results in database structural changes. We don't want to constantly be changing the field size every time they enter a new character.
• There should be an error state if they try to set a field size limit and they have values that are larger than the size they selected.

[pavish]

@ghislaineguerin

• I do not think we have other options for grouping, atleast until the MVP. So we could just show group by [column], rather than group [column] by [condition]. @kgodey, @mathemancer would we have other advanced options to group columns for the MVP?
• When a filter/sort is applied, there is absolutely no indication stating that there is a filter/sort applied. Until the user opens the filter pane, it is not visible. We should have visible indications of applied filter, sort and group on the top bar. I do not think they should be hidden within table actions.
• There should be an explicit notification and confirmation dialog and apply changes button for any changes in the advanced options sections. These operations modify the table structurally, so we cannot autosave them.

[kgodey]

Data type specific grouping actions are part of the MVP. Please see #234 for the design requirements for this.

[mathemancer]

I think the version looks good currently. The only (very minor) feedback I have is that sort by A to Z might be confusing if there are characters in the column that don't fit between A and Z. I'm not sure of a good solution, though, since having sort by 0 to % is probably more confusing.

[ghislaineguerin]

@mathemancer another option is to call them ascending or descending order, but I think A Z communicates the order more than the actual first and last items in the set

[ghislaineguerin]

@pavish @kgodey
I have updated the prototype based on the feedback. I think @pavish is right about the filter, sort, and group options. Initially, I wanted to show all of the options together so that when we build views and have table relationships visible, it would all be in a consolidated 'Create View' panel. However, as Pavish mentioned earlier, indicating that any of them is applied becomes harder with just the column header indicators. Now that the options are split into different panels, the sidebar makes less sense, and I'm tempted to try a different solution. However, I think that should be done as a revision to the original filter design rather than slowing down progress on data types. What do you think? @kgodey, I can also skip that change for now and introduce a new filter design for the next data type I'll be working on.

[kgodey]

@ghislaineguerin The new spec looks good. Some feedback:

(1) While filtering, rather than having separate dropdowns for "length" and ">=" between "release_name" and "255", can we just do one dropdown that says "is longer than"? It's easier to read/parse, I think.

The "Advanced Options" menu currently replaces the main column menu, which is confusing without context. It would be nice if it could be next to the main column menu or otherwise has more context about which column it's related to.

(2) This feedback from my previous comment is still unaddressed.

[ghislaineguerin]

@kgodey On the advanced options, we had discussed adding the column name to the menu. I can make it more visible.

[kgodey]

@ghislaineguerin I see that now. I think making it more visible (in the header of the modal?) would be easier to notice.

[ghislaineguerin]

@pavish @kgodey @mathemancer
I have updated the prototype based on feedback. Please review.

[kgodey]

@ghislaineguerin Looks good. A couple of comments:

• I thought of a shorter way to represent "greater than or equal to" in the filter, you could say "length is at least".
• I still think the name of the column should go somewhere else in the "Advanced Options" menu. If the column name is really long, it will make the panel very wide, especially if it's in the middle of two other words "Set" and "type". However, this is not specific to the text type so you might want to close review on this one and address it in a future type. You might end up redesigning that panel a bit because of my advanced options comment in Number Data Types #482 anyway.
• I like the filter dropdowns!

[ghislaineguerin]

@kgodey I've updated the filter options to "length is at least" and "length is at most"