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

mitosheet: add a csv config taskpane #382

Merged
merged 9 commits into from Jul 5, 2022
Merged

Conversation

naterush
Copy link
Member

@naterush naterush commented Jun 24, 2022

Description

Implements: https://www.notion.so/trymito/Import-Improvements-bb65727e1ba1479ea91ed176077fe589

A few questions to really polish this off:

  1. Do we want to case on the types of errors to display a different message? I wonder how well we might be able to do here, or is it not worth it.

Testing

Add something that causes an error to the path where we try to guess the delimeter, to see if it automatically opens the config.

Documentation

We should update our import documentation!

@vercel
Copy link

vercel bot commented Jun 24, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
monorepo ✅ Ready (Inspect) Visit Preview Jul 5, 2022 at 3:08PM (UTC)

@cla-bot cla-bot bot added the cla-signed label Jun 24, 2022
Copy link
Member Author

@naterush naterush left a comment

Choose a reason for hiding this comment

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

Cleanup! And test!

mitosheet/mitosheet/errors.py Outdated Show resolved Hide resolved
mitosheet/src/components/taskpanes/Import/CSVImport.tsx Outdated Show resolved Hide resolved
mitosheet/src/components/taskpanes/Import/CSVImport.tsx Outdated Show resolved Hide resolved
@aarondr77
Copy link
Member

aarondr77 commented Jun 30, 2022

I think since usually the button at the bottom is disabled anyways, we can also keep the configure button next to it. Instead of the config button appearing when this button becomes non-disabled, they both become non-disabled at the same time.

I guess it would be weird to display in the case that the user is looking at an excel file though. So down to leave it as is for now and note the difference in excel file and csv file handling in import is a bit of minor product debt.

@aarondr77
Copy link
Member

Is there enough space to add the configuration icon you created in Figma? It looks great!

Screen Shot 2022-06-30 at 4 36 10 PM

@aarondr77
Copy link
Member

Delimiter is spelled incorrectly here

Screen Shot 2022-06-30 at 4 38 22 PM

@aarondr77
Copy link
Member

I love that if you get taken to the config screen because you tried to import and it errored that we don't display the message that says: try to use Mito's guess, its good. lol

@aarondr77
Copy link
Member

aarondr77 commented Jun 30, 2022

Can we make this text change on hover so its more noticeable that you can click on it? And do the same with the other reset text in the bottom of the taskpane?

Screen Shot 2022-06-30 at 4 48 16 PM

EDIT: I removed this button, as it's redundant and I don't think it's really that useful in practice (if they want they default, they will likely take it, and if they don't want the default it's likely because it won't work). I improved the click ability of this at the bottom.

@aarondr77
Copy link
Member

aarondr77 commented Jun 30, 2022

This message is backwards. Turning it OFF does this.

Screen Shot 2022-06-30 at 4 49 08 PM

Maybe it would be more clear if we set it to Skip bad lines and the default mode was OFF and then we turned it ON when the user clicked it.

@aarondr77
Copy link
Member

If this message wasn't there when the taskpane opened because the user already tried the default params, it should remain not there.

Screen.Recording.2022-06-30.at.4.51.11.PM.mov

@aarondr77
Copy link
Member

The error message "We were unable to automatically determine a delimiter ..." should only be the error message the first time the taskpane loads. If error occured after they configured the taskpane, then it shouldn't reference automatically determining.

@aarondr77
Copy link
Member

Bug displaying the error message at the correct times:

  1. Attempt to import a file where you need to skip the bad lines. See the error message appear at the top of the taskpane.
  2. Use the toggle to skip the bad lines. Try to import. See that the file imports.
  3. Use the toggle to not skip the bad lines. Try to import. See that no error message appears in the taskpane, but an error is created in Jupyter.
Screen.Recording.2022-06-30.at.4.57.27.PM.mov

@aarondr77
Copy link
Member

aarondr77 commented Jun 30, 2022

Maybe not caused by this PR, but sometimes when I hit the back arrow in the excel import taskpane, the taskpane flashes to something else. Notice in the video it doesn't happen the first time, but does happen the second time.

Screen.Recording.2022-06-30.at.5.05.39.PM.mov

Copy link
Member

@aarondr77 aarondr77 left a comment

Choose a reason for hiding this comment

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

Code looks great. Just a few minor bugs to fix up then LGTM

mitosheet/mitosheet/api/get_csv_file_metadata.py Outdated Show resolved Hide resolved
mitosheet/src/jupyter/api.tsx Outdated Show resolved Hide resolved
@naterush
Copy link
Member Author

naterush commented Jul 4, 2022

Ok, this is ready for rereview. There is a bit of weirdness around when the messages at the bottom are displayed that I think is kinda silly (there are all these little messages appearing in and out). I think we should simplify (it confuses me as a user), while also not working exactly how we want b/c there are 3 pairwise interactions. My proposed changes:

  1. We reduce the "reset parameters" text at the buttom to a single line, and always display it.

I think this would make this much less jumpy and feel like you're missing less. Let me know what you think!

@aarondr77
Copy link
Member

aarondr77 commented Jul 5, 2022

Your proposal for the line at the bottom sounds good to me. Currenlty, the text appears at random times that make no sense to me.

@aarondr77
Copy link
Member

LGTM once you update the import documentation too!

@naterush
Copy link
Member Author

naterush commented Jul 5, 2022

We do documentation updating through the release process now :)

This is ready to merge but gonna wait until the next release, as I'm in the middle of testing rn.

@naterush naterush merged commit cb78bc4 into dev Jul 5, 2022
@marthacryan marthacryan deleted the import-error-reduction branch March 14, 2024 20:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants