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

Mobx dragdrop file fix #4139

Merged
merged 11 commits into from
Apr 20, 2020
Merged

Mobx dragdrop file fix #4139

merged 11 commits into from
Apr 20, 2020

Conversation

nf-s
Copy link
Contributor

@nf-s nf-s commented Mar 12, 2020

Fix file drag and drop, and fix "auto" file type resolving

Fixes #4102

I have fixed drag and drop, and 'auto' file type resolving

Checklist

  • Make auto file type work
  • Discuss if we should refactor the whole CreateCatalogItemFromFileOrUrl... thing This can be discussed at a later stage
  • Refactor if needed
  • Add test for 'auto' file type
  • I've updated CHANGES.md with what I changed.

@CLAassistant
Copy link

CLAassistant commented Mar 12, 2020

CLA assistant check
All committers have signed the CLA.

@na9da
Copy link
Collaborator

na9da commented Mar 12, 2020

@nf-s Merging from the mobx branch should fix the CI errors.

@nf-s
Copy link
Contributor Author

nf-s commented Mar 12, 2020

will do, thanks!

@rowanwins
Copy link
Contributor

You've got prettier errors @nf-s so another update required :)

It turns out husky (which should auto-run prettier for you when you commit code) only works well with certain versions of node so you might need to just double check that.

@nf-s nf-s added the Version 8 formerly MobX label Mar 18, 2020
@nf-s
Copy link
Contributor Author

nf-s commented Apr 9, 2020

Drag dropping in CSVs now works.

Changes:

  • Mobxified DragDropFile and DragDropNotification
  • sourceReference not being passed through the CsvCatalogItem's constructor. A Developer error was being thrown but was not printed to console for some reason (and the error's stack trace was not useful).
  • Change CsvCatalogItem's forceLoadTableData to load csvFiles before urls (I'm not sure of wider implications of doing this)
  • Renamed createCatalogItemFromUrl to createUrlReferenceFromUrl to clear confusion between createCatalogItemFromUrl and createCatalogItemFromFileOrUrl
  • After the UrlReference has been created in createCatalogItemFromFileOrUrl, the target is checked for hasFileInput (recursively)

The whole thing is a bit messy, but I'm not sure how to clean it up at this point:

  • Maybe create a FileInputMixin or LocalDataMixin
  • Or add some sort of fileInput to UrlReference
  • The whole isUrl and allowLoad thing is confusing
  • Should there be some separation of URL sources and file sources?

@nf-s nf-s marked this pull request as ready for review April 9, 2020 03:24
@nf-s nf-s requested a review from soyarsauce April 9, 2020 03:24
@nf-s
Copy link
Contributor Author

nf-s commented Apr 9, 2020

I'm not sure why 1) changes cursor to crosshair when entering drawing mode - UserDrawing that requires WebGL test is failing

@nf-s nf-s changed the title WIP Mobx dragdrop file fix Mobx dragdrop file fix Apr 9, 2020
@nf-s nf-s requested a review from rowanwins April 15, 2020 06:49
@nf-s nf-s removed request for rowanwins and soyarsauce April 17, 2020 07:49
@nf-s nf-s marked this pull request as draft April 17, 2020 08:21
@nf-s
Copy link
Contributor Author

nf-s commented Apr 17, 2020

Urgh - a test error:

should create an catalog item (CSVCatalogItem) from File (csv) without specifying a dataType
ReferenceError: 'fetch' is undefined

I love IE

@nf-s nf-s marked this pull request as ready for review April 20, 2020 01:22
@KeyboardSounds KeyboardSounds merged commit 2517704 into mobx Apr 20, 2020
@nf-s nf-s deleted the mobx-dragdrop-file-fix branch April 20, 2020 03: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.

5 participants