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

Handling Null Values Spec #13

Merged
merged 16 commits into from
Dec 22, 2021
Merged

Handling Null Values Spec #13

merged 16 commits into from
Dec 22, 2021

Conversation

ghislaineguerin
Copy link
Contributor

@ghislaineguerin ghislaineguerin commented Nov 25, 2021

This PR adds the specs to implement the design for UX to allow user to set value to NULL within a cell

@ghislaineguerin ghislaineguerin marked this pull request as ready for review November 25, 2021 23:16
@ghislaineguerin ghislaineguerin requested a review from a team as a code owner November 25, 2021 23:16
@github-actions github-actions bot requested review from eito-fis, kgodey, mathemancer, pavish, seancolsen and silentninja and removed request for a team November 25, 2021 23:16
@kgodey kgodey added the status: review In review label Nov 26, 2021
@kgodey
Copy link
Contributor

kgodey commented Nov 26, 2021

@dmos62 I unassigned you since you approved the spec.

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 Please also add a link to this spec from the main specs page.

design/specs/specs-null-values.md Outdated Show resolved Hide resolved

1. The user edits a NULL value in a column that is set to text data type
2. The user double-clicks the value cell and enters edit mode
3. The user doesn't enter a value and leaves edit mode by hitting enter or clicking outside of the active cell
Copy link
Contributor

Choose a reason for hiding this comment

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

I know we talked about this already in the discussion, but now that I see it in action in the video, I'm not sure about it. Sometimes, when I'm working with spreadsheets or tables, I just select a cell to highlight it for myself to help me think. In this case, if a user highlights the cell and stops highlighting it, then the value of that cell in the database will change, which I don't think users intend to do.

Instead, I think that we should only set the cell to an empty string if the user types something and then deletes it, not just if they enter edit mode and leave it. If they enter edit mode and leave it, it should stay NULL.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a differentiation between "selecting" a cell, and "edit mode" on a cell? For me, that's the most natural, and then this would be less of an issue. @ghislaineguerin mentions "double-clicking" to enter edit mode, which is similar to how spreadsheets work (at least to my recollection). If that's the case, it might make sense to somehow make the fact that you've entered "edit mode" very obvious and intentional. For example, with LibreOffice, the cell highlight changes color and there's also an edit box in another place that activates when you're in "edit mode", but when you're just cursoring around, those things don't happen. Just an idea.

I agree that we should try to make it an intentional act to set the cell to an empty string, though. It's more normal to leave things as null than empty strings for most DB schemata.

Copy link
Contributor

Choose a reason for hiding this comment

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

we should only set the cell to an empty string if the user types something and then deletes it, not just if they enter edit mode and leave it. If they enter edit mode and leave it, it should stay NULL

I agree.

I think it will be rare for the user to intentionally change a cell value from NULL to empty string. However it will be more common for users to begin editing a cell (perhaps on accident) then stop editing that cell. We should avoid assigning a rarely desired outcome to a more commonly performed action.

Further, I think that when the user enters edit mode for a cell with a null value, the cell should look different that it would with an empty string, even in edit mode. I think we should still display "NULL" (in italic) and place the user's cursor at the start of the text. Then when the user begins to type, the "NULL" text will disappear.

Converting a NULL value to an empty string would look like this:

  1. Observe NULL in gray italic.
  2. Double click.
  3. Cell indicates the user is in edit mode by displaying thick border.
  4. Cell indicates its value is NULL by displaying NULL in gray italic.
  5. Cell places the user's cursor furthest to the left, before the NULL indicator text.
  6. User enters any character, e.g. "a".
  7. Cell displays "a". The NULL text disappears.
  8. User presses Backspace.
  9. Cell displays as empty -- no characters, no NULL indicator.
  10. User presses Enter or blurs focus.
  11. Cell exits edit mode and the value is saved as an empty string.

Copy link
Member

Choose a reason for hiding this comment

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

I think everyone have covered this concern very well. I am on agreement with @seancolsen's suggestion to show null even during edit mode when user has not begun to edit yet.

@kgodey kgodey removed their assignment Dec 8, 2021
@silentninja silentninja removed their assignment Dec 9, 2021
@silentninja
Copy link
Contributor

silentninja commented Dec 9, 2021

@ghislaineguerin It would be great if you can document the null behaviour for dropdown type cells?

@kgodey
Copy link
Contributor

kgodey commented Dec 9, 2021

@ghislaineguerin Please re-assign me when the spec is ready for re-review.

Copy link
Member

@pavish pavish left a comment

Choose a reason for hiding this comment

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

@ghislaineguerin I think this looks good but keyboard interactions are not detailed anywhere, which would usually be the primary way of doing things.

@kgodey I do not think we should be spending more time on this at the moment, the spec looks good for mouse based interactions.

Perfecting this requires having to play around with the UI for all our usecases (including checkboxes, dropdowns, calendars and other input types), and we do not have visibility until we first figure out how data entry works on each of these types (using both keyboard and mouse interactions).

Setting cells to NULL using keyboard interactions for each individual scenario/cell type should be decided after we decide how to enter data rapidly on each of these cell types, which should probably be after our data types milestone is complete. For the time being I think we can work on just the mouse based interactions.

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.

I'm confused about edit mode vs. select mode. I don't think we should require multiple clicks to edit a cell because it will be harder to do rapid data entry.

##### Steps for 2a

1. The user edits a `NULL` value in a column set to text data type.
2. The user double-clicks the value cell to enter edit mode.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confused about this - I thought we had consensus in the comment thread that we should only require a single click to enter edit mode?

See these three comments:

Copy link
Contributor

@silentninja silentninja Dec 16, 2021

Choose a reason for hiding this comment

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

@kgodey When you click on a cell once, the cell gets selected (ie) it would be a greyed-out box without a cursor with a blue highlight. But for convenience, when the user starts typing(any keystroke other than delete), the cell will change the mode, so it enters into edit mode with the cell turning into a text box, similar to what would have happened if you had double-clicked the cell

Copy link
Contributor

Choose a reason for hiding this comment

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

@silentninja That makes sense to me, but then this part (later in the spec) becomes confusing to me:

While in Select Mode, pressing the delete or backspace key will delete the cell state and convert it to a NULL value.

I would expect that if typing converts it to edit mode, delete/backspace will remove a single character, not convert it to NULL. It seems inconsistent to have special behavior for one key (that's also used in regular typing).

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 think what @silentninja explained should also be explained better in the spec.

Copy link
Contributor

@seancolsen seancolsen Dec 16, 2021

Choose a reason for hiding this comment

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

I agree with @silentninja except for "it would be a text box awaiting a text input rather than a greyed-out box without a cursor" (I find that part confusing).

Here's my take on how a spreadsheet works and also how Mathesar should work:

current mode pointer action pointer target result
select single click current cell nothing
select single click different cell move selection to new cell
select double click current cell switch to edit mode
select double click different cell move selection to new cell, switch to edit mode
select triple click current cell switch to edit mode, select all
select triple click different cell move selection to new cell, switch to edit mode, select all
edit single click current cell set cursor position
edit single click different cell save changes to current cell, enter select mode, move selection to new cell
edit double click current cell select word under cursor
edit double click different cell save changes to current cell, move selection to new cell, remain in edit mode

Now let's add in the Delete and Backspace behavior

current mode DOM key result
select any text character switch to edit mode, set cell contents equal to character entered
select Backspace set cell value to NULL
select Delete set cell value to NULL
select Shift+Backspace nothing
select Shift+Delete nothing
edit Backspace delete character to the left of cursor
edit Delete delete character to the right of cursor
edit Shift+Backspace discussion pending
edit Shift+Delete discussion pending

Copy link
Contributor

Choose a reason for hiding this comment

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

Undo has always been high on my mental prioritization for that reason - I don't think users will be able to fully explore and discover features without being able to fix mistakes. But it's also complicated to implement so I think we made the right decision to push it to post-alpha.

Copy link
Contributor

@seancolsen seancolsen Dec 17, 2021

Choose a reason for hiding this comment

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

@kgodey I had a few more thoughts on this thread last night.

  • @ghislaineguerin I'm curious to hear your opinion here. To summarize, we've preliminarily agreed to follow Kriti UX which deviates from standard spreadsheet UX as follows

    Current mode DOM key Spreadsheet UX Kriti's UX
    select any text character switch to edit mode, replace cell contents with character switch to edit mode, place cursor at end, append character to end of cell contents
    select Backspace delete cell contents switch to edit mode, place cursor at end, delete last character

    I'd like us to adhere to the spreadsheet UX, but wait until we have implemented undo before we implement those actions. However, for the sake of moving forward, I've agreed to Kriti's proposal.

    I just want to make sure we're not losing your voice here, Ghislaine since you're the UX person!

  • Kriti said:

    I'd rather take a risk on this new pattern in our alpha release to see if it works well for users. If it doesn't, then we can always switch to the well-established spreadsheet pattern.

    This had me thinking. If we take a risk on a UX pattern that is unfamiliar to users, I think we're going to need to be very aware of this divergence and intentionally seek feedback on it during our user testing. I doubt we'll have the resources to dedicate towards the type of user testing the Microsoft has done with Excel or that Google has done with Sheets. I'm sure those companies have run paid focus groups to observe new users interacting with the product. In my experience building UIs, it's absolutely essential to watch users with the product when seeking feedback on theses subtle UX decisions, but it takes time, especially with a large product. New users will basically never express opinions with the specificity necessary for actionable change on small UX decisions. But watching them use the product will inevitably uncover problems. If we diverge from well established UX patterns that type of focus group testing becomes even more important so we'll need to keep this in mind and consider allocating some budget towards it later this year.

Copy link
Contributor

Choose a reason for hiding this comment

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

In my experience building UIs, it's absolutely essential to watch users with the product when seeking feedback on theses subtle UX decisions, but it takes time, especially with a large product. New users will basically never express opinions with the specificity necessary for actionable change on small UX decisions.

It's always been my plan to schedule sessions with users where we watch them use the product after release. I have some experience with running user tests, I read Rocket Surgery Made Easy at my last job, which is a great guide to usability testing and I highly recommend it.

Copy link
Contributor Author

@ghislaineguerin ghislaineguerin Dec 21, 2021

Choose a reason for hiding this comment

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

@kgodey I tend to agree with @seancolsen about not introducing a different interaction pattern when we can stick with spreadsheet patterns and focus on reducing data loss instead. Selecting and appending characters to the end is not a pattern I've encountered so far. It might become annoying if users work across different tools while using Mathesar. I believe tests with users will be best for understanding entire flows and steps towards different goals rather than isolated interactions. I get your point, but from my experience, these friction points aren't apparent during testing, as they become worse once the user has to switch between multiple tools that have inconsistent behavior.

I prefer that we stick to the spreadsheet pattern and minimize the potential issues caused by the lack of UNDO.

Appending a character to the end would also be inconsistent with how we plan to transform content into NULL. In which case, we'd have only to remove the last character of content rather than replace it entirely with a NULL value.

Copy link
Contributor

@kgodey kgodey Dec 22, 2021

Choose a reason for hiding this comment

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

@ghislaineguerin and I talked about this on a call, we'll deal with this in the follow up issue mathesar-foundation/mathesar#917.

@kgodey
Copy link
Contributor

kgodey commented Dec 16, 2021

I'm fine with deferring keyboard interactions as @pavish suggested, @pavish or @ghislaineguerin could one of you create an issue so that we don't forget about it?

@ghislaineguerin
Copy link
Contributor Author

@kgodey I've updated the spec. I will create the additional issue for select and edit mode.

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 Added some comments and suggestions for some minor missing items. Please feel free to merge after you fix them, no need for an additional review.

Also, I haven't reviewed the Loom videos to see if there are any changes needed. Could you please review them and make sure they follow the same behavior as described in the spec? We've changed some things based on discussions since the videos were recorded.

design/specs/specs-null-values.md Outdated Show resolved Hide resolved

1. The user selects a cell in a column that contains a value.
2. The user clears the content of the selected cell.
- By pressing the `delete` key.
Copy link
Contributor

Choose a reason for hiding this comment

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

We decided to make this shift-delete instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

or shift-backspace.


1. The user selects a cell in a column that contains a value.
2. The user attempts to clear the content of the selected cell.
- By pressing the `delete` key.
Copy link
Contributor

Choose a reason for hiding this comment

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

We decided to make this shift-delete or shift-backspace instead.


### Select vs. Edit Mode Behaviors

In `Edit Mode`, a cell provides additional functionality and interactions to users. `Edit Mode` is toggled by double-clicking on a cell. However, certain data types could have different interactions. Once specific behaviors and components for each data type are introduced, we must further define these interactions to ensure compatibility.
Copy link
Contributor

Choose a reason for hiding this comment

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

We decided to also enable Edit Mode if the user types a text character or backspace while in Select Mode.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would be good to also define Select Mode since it's referenced in the table below.

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.

None yet

8 participants