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

Record selector bug fixes and UX improvements #1856

Merged
merged 82 commits into from
Nov 11, 2022
Merged

Conversation

seancolsen
Copy link
Contributor

@seancolsen seancolsen commented Oct 20, 2022

This PR

Demo of some notable design changes

2022-11-03.13-56-26.mp4

Remaining issues to handle later

What's next

  • After merging this PR, I'd like to invite the whole team to hammer at the Record Selector a bit to see if we can uncover any remaining bugs or UX issues. As such, I don't want to hold up this PR due to small things we can fix in that next phase of polish.

Checklist

  • My pull request has a descriptive title (not a vague title like Update index.md).
  • My pull request targets the master branch of the repository
  • My commit messages follow best practices.
  • My code follows the established code style of the repository.
  • I added tests for the changes I made (if applicable).
  • I added or updated documentation (if applicable).
  • I tried running the project locally and verified that there are no visible errors.

Developer Certificate of Origin

Developer Certificate of Origin
Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

@seancolsen seancolsen changed the title Record selector UX improvements Record selector bug fixes and UX improvements Nov 3, 2022
@seancolsen seancolsen marked this pull request as ready for review November 3, 2022 18:17
@seancolsen seancolsen added the pr-status: review A PR awaiting review label Nov 3, 2022
@seancolsen
Copy link
Contributor Author

@kgodey and @ghislaineguerin You can look at the demo video here for a tour of the UX design changes to the Record Selector.

@kgodey
Copy link
Contributor

kgodey commented Nov 4, 2022

@seancolsen I don't think I'll be able to watch the video in full in the next couple of days (I don't yet have the necessary focus), but I skimmed through it and from what I see, the changes look great. I'm looking forward to playing around with it once it gets merged.

Comment on lines +35 to 39
options: [null, true, false],
getLabel: (value?: boolean | null) => {
if (isDefinedNonNullable(value)) {
return value ? labels[0] : labels[1];
}
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to allow null here? If we do, we should represent Null using the NULL component instead of empty string.

While we can allow null, there's no way for any other components (checkbox, text boxes, textarea etc.,) to ever set null as a value, so this would be a special option allowed only by the Select menu within a cell.

Copy link
Contributor Author

@seancolsen seancolsen Nov 10, 2022

Choose a reason for hiding this comment

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

Do we need to allow null here?

The reason for this change is to improve the behavior of the input for boolean columns within the record selector. Before this change, the input only had options for "true" and "false". The records selector needs to open with blank values for all inputs, and the user needs to be able to set a value and then set it back to some sort of blank value after setting it. If you can think of a better way to accomplish what I'm after, then I'm open to changing the approach here.

we should represent Null using the NULL component instead of empty string.

I'm not sure about this. I think that would look pretty weird.

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 that would look pretty weird.

I agree this would probably look weird in the context of record selector search. But we use the same component for the Cells in the table, and we represent nulls using the Null component in the Cell. The empty value looks odd in that context.

Either way, this is not a priority at the moment, and we can figure this out later.

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 just tested a boolean column. The dropdown shows an empty option. When you select the empty option the cell value changes to NULL and displays stylized. I think that should suffice. It would be nice to hide the empty option of the column doesn't allow NULL, but I don't imagine that's a priority.

Though it's worth noting that I did uncover #1919 while checking this out, and I predict that's a regression from this PR too. But I think it's worth merging this in and fixing that subsequently.

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.

@seancolsen Looks good! Nice work!

I'll leave reviewing of the UX to @kgodey and @ghislaineguerin, which can probably be done post merge.

I noticed a couple bugs while doing a quick local test:

  1. Regression: Array type column styling is broken. Open Data Explorer and summarize any column so that it results in an Array type column. Notice that the css modifications result in this column being messed up. I'd like this to be fixed before merging the PR in.
    Screenshot 2022-11-10 at 11 59 15 PM

  2. Selection is not reset after opening & closing record selector. Click on a column that opens Record selector, and use the keyboard to pick a row. Notice that the record selector is closed. Move the mouse around without clicking and notice that the cells are being selected when the mouse is moved, which is not desired. Feel free to create an issue for this and handle it later if you do not wish to address it in this PR.

    Screen.Recording.2022-11-11.at.12.02.10.AM.mov

Comment on lines 67 to 71
function setRecordSelectorToAccompanyDropdown() {
const modal = document.querySelector<HTMLElement>('.modal-record-selector');
if (dropdownAccompanyingElements && modal) {
dropdownAccompanyingElements.add(modal);
}
Copy link
Member

@pavish pavish Nov 10, 2022

Choose a reason for hiding this comment

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

The modal element that's added here is never removed. While the modal itself is removed from the DOM, it's reference will be held here until the parent AttachableDropdown is removed.

While this should cause no major problems, I'm certain we'd have to do similar operations in other places and it would be better if we ensure that we always remove stale references.

Not a priority. We can keep track of this to handle later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it would be better if we ensure that we always remove stale references.

Fair enough. I added 4f01720 to address this.

@pavish
Copy link
Member

pavish commented Nov 10, 2022

@seancolsen I noticed another bug:

Screen.Recording.2022-11-11.at.12.12.14.AM.mov
  • Open the Record selector
  • Use keyboard to navigate to a column which inturn opens up another record selector modal
  • Pick a value by using the arrow keys and enter key, notice that it gets selected as expected, and the second modal closes
  • Notice that the first modal now has filtered values based on the selection
  • Use the arrow keys to highlight the row you'd like to pick and use enter key to select it
  • Expect the row to get selected and the modal to close, instead observe that the second modal opens up again

I'll leave it upto you to decide if you'd like to fix this in this PR, or separately (in which case, please create an issue), or maybe later during the post-release improvements.

@pavish pavish assigned seancolsen and unassigned pavish Nov 10, 2022
@pavish pavish added pr-status: revision A PR awaiting follow-up work from its author after review and removed pr-status: review A PR awaiting review labels Nov 10, 2022
@seancolsen
Copy link
Contributor Author

@pavish this is ready for re-review.

@seancolsen seancolsen added pr-status: review A PR awaiting review and removed pr-status: revision A PR awaiting follow-up work from its author after review labels Nov 10, 2022
@seancolsen seancolsen assigned pavish and unassigned seancolsen Nov 10, 2022
@pavish pavish merged commit 32b5f7b into master Nov 11, 2022
@pavish pavish deleted the record_selector_ux branch November 11, 2022 07:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-status: review A PR awaiting review
Projects
No open projects
4 participants