-
-
Notifications
You must be signed in to change notification settings - Fork 320
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
Display null while editing a null cell #961
Display null while editing a null cell #961
Conversation
Thanks for this PR, @gagan2005. I'll review it sometime within the next few days. |
@kgodey can you help troubleshoot the failing test in this PR? |
@seancolsen It looks like the automatic add-to-project failed because this is based on a fork. I updated the GitHub action event to compensate in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gagan2005 Thanks for this PR!
Using the placeholder
attribute is a clever quick-fix, but this approach has some shortcomings particular to our needs here.
- In step 6 of the desired behavior in Display NULL while editing a NULL cell #939, we need to show an empty cell instead of the placeholder. Your changes effectively satisfy that goal, but only as a consequence of Wait until user is done editing a cell before saving #928 still being unresolved. You can simulate the behavior we're aiming for with that fix by commenting out the contents of
debounceAndSet
withinRowCell.svelte
. When the cell doesn't auto-save itself, then the placeholder continues to display after typing text into a NULL cell and then subsequently deleting that text. - On a slightly lower-priority stylistic note, I'd like to style the NULL value the same during edit mode as it is styled during select mode. Apologies that I did not include a note about that in the original ticket. I've edited it now to mention this additional goal, and I've linked to a What's the best terminology to use in our UI for the concept of NULL? #778 where we discussed some of the reasons to display NULL as stylized wherever possible.
With the above two considerations in-mind. I'd recommend that instead of using the placeholder
attribute, we conditionally render a Null
component (already exists) and absolutely position it within the cell. Would you be up for taking another stab at this issue using that approach?
ok , I will make changes and Update this PR |
@gagan2005 How's it going on this? Are you still planning to work on it? Can I help with anything? |
Sorry, could not work on it as I have been busy last few days. I will resume the work today and inform you if I need any help |
@seancolsen Earlier everything was fine but Now I can't get docker-compose up to work. It says arrow module not found. I checked the dockerfile, it is using requirements.txt and the file has arrow module listed. I am unable to figure out what might be causing the issue |
@gagan2005 You might need to run Sometimes (though rarely) you'll need to re-build the container if after pulling in other changes. If you're still having trouble, post a message in the "Code - General" Matrix channel with the specific error message you're seeing. |
83f6ed2
to
53b0c3d
Compare
@seancolsen Please take a look and inform me if any other changes are required. |
Codecov Report
@@ Coverage Diff @@
## master #961 +/- ##
=======================================
Coverage 92.46% 92.46%
=======================================
Files 89 89
Lines 3200 3200
=======================================
Hits 2959 2959
Misses 241 241
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gagan2005 Thanks for continuing to work on this.
I like that I can see NULL
stylized now during edit mode. However, we still need to address a few more things before merging this.
Even though we're not using the placeholder
functionality (like you originally did), we're essentially trying to mimic it with a stylized placeholder that permanently disappears once the user has modified the value of the input. In other respects though, we are aiming for a UX that mirrors the placeholder behavior because users are familiar with it. As such, we need to make the following changes to this PR:
-
Keep the
NULL
placeholder when the user presses a key that does not modify the value of the input.For example, if the user presses a key like
Up
,Home
,PgDown
,Delete
,Shift
when the placeholder is shown, the placeholder will disappear. There are a lot of keys like that, and we don't want to have to list them all. Instead of thekeydown
event, we should probably be listening to theinput
event to determine the appropriate point at which we can remove the placeholder. -
Display the user's cursor before the placeholder, not after.
Instead of shifting the input with
left: 54px;
, I'd recommend that we keep its current position and conditionally render an additional<Null />
component absolutely positioned on top of the input.
Thanks @gagan2005 This is working well now. I added 0a60b17 to simplify the |
Fixes #939
The cells which have null values will now show null when we edit the value in the cell.Previously the cell will show up as empty when we start editing it
Technical details
I added a flag to check whether NULL needs to be displayed or not. When we need to display null , we shift the Input behind the null element. If user enters any input the flag is set to false and the input element shifts back to in front of null element
Screenshots
Checklist
Update index.md
).master
branch of the repositoryvisible errors.
Developer Certificate of Origin
Developer Certificate of Origin