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.

Sign up for GitHub

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

Jump to bottom

Update importers to use a single dialog #2685

Merged
merged 12 commits into from
Mar 18, 2021
Merged

Update importers to use a single dialog #2685

merged 12 commits into from
Mar 18, 2021

Conversation

sandymcfadden
Copy link
Contributor

@sandymcfadden sandymcfadden commented Feb 22, 2021 •
edited
Loading

Fix

First PR to look at #2570. This is starting to combine the importers to one Dialog so that any of the supported file types can be chosen and imported. This also now allows the ability to import more than one Simplenote or Evernote export file at the same time.

Screenshots

Light mode select files Screen Shot 2021-03-08 at 10 23 46 AM
Light mode files added Screen Shot 2021-03-17 at 12 20 50 PM
Light mode complete Screen Shot 2021-03-17 at 12 20 19 PM
Dark mode select files Screen Shot 2021-03-08 at 10 28 45 AM
Dark mode files added Screen Shot 2021-03-17 at 12 19 22 PM
Dark mode complete Screen Shot 2021-03-17 at 12 19 54 PM

Test

  • Go to Settings > Tools > Import Notes.
  • Notice you can select any supported files.
  • Choose multiple combinations of txt, md, Simplenote, and Evernote files.
  • Ensure they import as expected.
  • Ensure selecting to enable markdown for all notes does enable markdown.
  • Smoke test in light and dark mode to ensure styling looks correct.

Release

  • Updating imports to a single dialog where any supported file type can be imported at the same time.
  • Updating the styles of the import dialogs.

@sandymcfadden sandymcfadden self-assigned this Feb 22, 2021
@sandymcfadden sandymcfadden changed the title [WIP] Update importer single dialog [WIP] Update importers to use a single dialog Feb 23, 2021
@sandymcfadden sandymcfadden mentioned this pull request Mar 4, 2021
Merged
dmsnell
dmsnell reviewed Mar 5, 2021
View reviewed changes
Copy link
Member

@dmsnell dmsnell left a comment

Choose a reason for hiding this comment

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

what is left in this @sandymcfadden that you think needs work? is it ready? are you happy with it? what would you like from review?

sandymcfadden added a commit that referenced this pull request Mar 5, 2021
@sandymcfadden
Updating general dialog styles to match new design (#2718)
c575a57 
There are a couple of other PRs (#2685 and #2653) that are in the works that touch the dialog styles. Instead of making changes in them individually, I'm pushing this to make the general changes to dialogs, and then the individual ones can get any overrides they need.
@sandymcfadden
Starting out by loading a single importer rather than a choice dialog.
ebf1ea4 
Updating the styling of the importer modal to start matching the designs

adding in cloud icon

Adding in the multiple importer which can handle any files supported by our importers.

Adding list of accepted files once selected to upload, and roughly updating styles.

Add error message for when multiple Simplenote or Evernote files are added

Fixing lint error and changing a let to a const

Updating styles for the import dialog in light mode

update importer progress styles

Bit more style updates to the importer progress

Start updating the styles for the dark theme

Updating some more styles for dark theme

Remove a couple of no longer needed files

Clean up code in the dropzone to be less repeative.

Remove characters from tag names which aren't allowed

Cleaning up and removing some duplicated code

Add file to keep the importers together

Clean up and remove duplicate code

Refactoring and cleaning up error messages based on feedback

Update error handling and refactor the importers

Fix error message display bug where it would display 0 when no errors

Add analytics for each importer

Updating text importer analytics to match previous source name
@sandymcfadden
Updating strings for Import Files when single or plural is needed
320fb78
@sandymcfadden sandymcfadden force-pushed the update/importer-single-dialog branch from 26812d7 to 320fb78 Compare March 5, 2021 12:24
@sandymcfadden
Updating styles and working to allow multiple simplenote and evernote…
604e43d 
… import files at once.
@sandymcfadden
Updating multiple importer to allow importing multiple Simplenote and…
3c62793 
… Evernote files. Removing bug that caused duplicate evernote notes to be imported, thanks to Dennis
@sandymcfadden
Remove now unneeded properties on the importers and send oen file at …
8f52a8a 
…a time to all importers
@sandymcfadden sandymcfadden changed the title [WIP] Update importers to use a single dialog Update importers to use a single dialog Mar 8, 2021
@sandymcfadden sandymcfadden marked this pull request as ready for review March 8, 2021 15:10
@sandymcfadden sandymcfadden requested a review from dmsnell March 8, 2021 15:10
@sandymcfadden
Copy link
Contributor Author

@dmsnell this is ready for review now. I've tested on all platforms and all seems to be working as expected.

@dcalhoun dcalhoun mentioned this pull request Mar 9, 2021
Merged
@belcherj
Copy link
Contributor

Something feels wrong about the UI here, it seems too bunched up:

Screen Shot 2021-03-10 at 11 25 46 AM

@sandymcfadden
Copy link
Contributor Author

Thanks @belcherj I believe I have it matching the designs. Except for the elevation colors which I'll be working on in a separate PR as part of the UI spring cleaning.

@SylvesterWilmott what are your thoughts.

@belcherj
Copy link
Contributor

Looks like it matches designs, seemed a bit squooshed.

@sandymcfadden sandymcfadden added this to the 2.9.0 milestone Mar 11, 2021
@SylvesterWilmott
Copy link
Contributor

This looks great @sandymcfadden, thanks!

A few points of feedback, mostly related to the dialog component itself

  1. I think we need to include a max-height for the dialog component to avoid times like these where it can grow too tall. I would suggest 420px which will also be the width of our new larger dialogs eg. settings.
  2. I don't think dialogs should be dismissed when clicking outside of it. These dialogs are mostly accessible from the settings and so accidentally closing one can be quite frustrating. In my opinion, the "x" is a good enough click target to close them. What do you think?
  3. It looks like you've grabbed the file icon from the designs which is fine for now but we do need our own in Simplecons. I'll keep you updated on that, hopefully I can do that before this PR gets merged.

@sandymcfadden
Copy link
Contributor Author

Thanks so much, @SylvesterWilmott.

  1. I did have a max height set but it was pretty large. I basically made sure it didn't go outside of the screen size. I've updated that now to be the 420px you've suggested.
  2. That sounds good to me. I've updated the dialogs so they don't close by clicking outside of them.
  3. We had an existing File icon in the app already and I used it. I can certainly update it with a new one. Just let me know 🙂

@SylvesterWilmott
Copy link
Contributor

Looks great @sandymcfadden

One last thing, in dark mode, can we use SNBlue30 for the "Browse" text?

@SylvesterWilmott
Copy link
Contributor

I also wonder if in these dialog components, the bottom area containing the button should be in fixed position. Perhaps something to iterate on in a separate PR.

@sandymcfadden
Copy link
Contributor Author

sandymcfadden commented Mar 16, 2021 •
edited
Loading

Thanks @SylvesterWilmott! I've updated the browse color in dark mode now.

I also wonder if in these dialog components, the bottom area containing the button should be in fixed position. Perhaps something to iterate on in a separate PR.

I had almost asked you about this and thought the same thing. I think I'll go through in a separate PR and update all the dialogs to act that way.

Edit: Opened issue here #2777

dmsnell
dmsnell approved these changes Mar 18, 2021
View reviewed changes
Copy link
Member

@dmsnell dmsnell left a comment

Choose a reason for hiding this comment

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

It appears that this is largely in the styling stage. As long as no major code changes occur I'm happy with what's in this PR. This approval means "no need to check back with me; merge when the styling issues are resolved and you are satisfied with the state of the branch."

@sandymcfadden sandymcfadden merged commit 3bb8d23 into develop Mar 18, 2021
@sandymcfadden sandymcfadden deleted the update/importer-single-dialog branch March 18, 2021 17:10
@pachlava
Copy link
Contributor

pachlava commented Mar 24, 2021 •
edited
Loading

Tested on 2.9.0-beta1 under different setups (Windows 10 Home + Chrome, Edge, Desktop | macOS 11.2.3 Chrome + Desktop)

✔️ Importing md, enex, txt and json types simultaneously
❌ Importing 'alien' json (created #2806)
✔️ Selecting both supported and unsupported files types imports only the supported ones
✔️ Import via drag and drop as well as via browsing
✔️ Drag and drop of folder imports all supported files in this folder
✔️ Drag and drop of enex file to Web app is handled right (forbidden)
✔️ Using both light and dark themes
❌ Import of big number of files creates minor layout issue (created #2801)
✔️ Checking that Enable Markdown for all notes really works

@sandymcfadden sandymcfadden mentioned this pull request Mar 29, 2021
Merged
@sandymcfadden sandymcfadden mentioned this pull request Sep 28, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dcalhoun dcalhoun dcalhoun left review comments

@dmsnell dmsnell dmsnell approved these changes

@belcherj belcherj belcherj approved these changes

@SylvesterWilmott SylvesterWilmott SylvesterWilmott approved these changes

Assignees

@sandymcfadden sandymcfadden

Labels
None yet
Projects
None yet
Milestone
2.9.0
Development

Successfully merging this pull request may close these issues.
6 participants
@sandymcfadden @belcherj @SylvesterWilmott @pachlava @dcalhoun @dmsnell