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

Import/export saved favorites #964

Merged
merged 14 commits into from
Sep 12, 2019
Merged

Conversation

huboneo
Copy link
Contributor

@huboneo huboneo commented Sep 2, 2019

This PR heavily refactors the user favorites implementation, and adds support for importing/exporting saved scripts as a ZIP archive

Key changes

  • Replaces local user favorites implementation with a new ui kit package @relate-by-ui/saved-scripts (Bugfix saved scripts folder update neo4j-devtools/relate-by-ui#2)
  • Improves appearance and UX of favorites as per design requests
  • Updated webpack conf to support symlinked npm packages (for development)
  • adds ability to import and export user favorites as .zip archive

Unrelated changes

  • Addresses 2 bugs introduced by :param v2 #962
    • Added babel transpilation of node_module as IE does not support lambdas
    • Removed duplicate cmd chars in error and disconnected banners

Screenshots

Adding new favorites
favorites-1
Renaming favorites
favorites-2
Exporting favorites
favorites-3
Importing favorites
favorites-4

Running this PR locally

Todo

  • Add screenshots
  • Add unit tests for file-drop.utils
  • Run existing tests once new version of @relate-by-ui/saved-scripts is published

…ated jest.config for lodash-es and react-dnd
- Replaced existing favorites implementation with new components from @relate-by-ui/saved-scripts
- Added ability to export and import saved favorites using .zip
- Added @relate-by-ui/css for icons and material-ui styles
@@ -171,6 +194,10 @@ const mapDispatchToProps = dispatch => {
saveCypherToFavorites: file => {
dispatch(addFavorite(file))
},
saveManyFavorites: favorites => {
const folders = getFoldersFromFavorites(favorites)
console.log('saveManyFavorites', favorites, folders)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops, this should be calling LOAD_FAVORITES action creator

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed!

content: AboutDrawer
}
]
function Sidebar (props) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since class only had a render() method anyhow...

@@ -60,7 +65,7 @@ export default function reducer (state = initialState, action) {
return mergeFavorites(initialState, updatedFavorites)
case LOAD_FAVORITES:
case UPDATE_FAVORITES:
return mergeFavorites(initialState, action.favorites)
return mergeFavorites(action.favorites, state)
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 had to change this to ensure favorites got updated. Still not sure how it worked before, or if this is right

Copy link
Member

Choose a reason for hiding this comment

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

Yes, this is strange.

@huboneo huboneo force-pushed the feature-export-favorites branch from 942ecec to 5dc7e2a Compare September 6, 2019 08:41
Copy link
Member

@oskarhane oskarhane left a comment

Choose a reason for hiding this comment

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

I like the new looks of the favorites sidebar.
And I love that I can rename them from the UI now!

The syncing seems to be working perfectly, well done on that part.

A few things that should be addressed before this goes in (apart from eventual inline comments):

  1. Import doesn't seem to be working in Safari (I have not tried in IE 11 or FF). JS Error is thrown in console.

  2. Re-ordering of favorites does not seem to work anymore. Moving between folders seems to be working though.

@@ -98,7 +98,7 @@ module.exports = [
use: ['style-loader', 'css-loader']
},
{
test: /\.svg$/,
test: /\.(svg|png)/,
Copy link
Member

Choose a reason for hiding this comment

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

This break the neo4j logo on :play start. And possibly more places.

Copy link
Member

Choose a reason for hiding this comment

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

This is still an issue.
oskarhane-mbpt 2019-09-12 at 08 49 45

"deepmerge": "^2.1.1",
"dnd-core": "^2.5.1",
"dompurify": "^1.0.11",
"file-saver": "^1.3.8",
"firebase": "^5.8.3",
"isomorphic-fetch": "^2.2.1",
"jsonic": "^0.3.0",
"jszip": "^3.2.2",
Copy link
Member

Choose a reason for hiding this comment

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

We need to verify that this works in IE 11.

getMissingFoldersFromNames
} from './file-drop.utils'

describe('file-drop.utils', () => {
Copy link
Member

Choose a reason for hiding this comment

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

👍

updateFolder
} from './favorites.utils'

describe('favorites.utils', () => {
Copy link
Member

Choose a reason for hiding this comment

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

👍

@@ -60,7 +65,7 @@ export default function reducer (state = initialState, action) {
return mergeFavorites(initialState, updatedFavorites)
case LOAD_FAVORITES:
case UPDATE_FAVORITES:
return mergeFavorites(initialState, action.favorites)
return mergeFavorites(action.favorites, state)
Copy link
Member

Choose a reason for hiding this comment

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

Yes, this is strange.

import { SLASH } from './export-favorites.constants'
import { mapOldFavoritesAndFolders } from './export-favorites.utils'

describe('user-favorites.utils', () => {
Copy link
Member

Choose a reason for hiding this comment

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

👍

@huboneo
Copy link
Contributor Author

huboneo commented Sep 11, 2019

@oskarhane thank you for your feedback!

  1. Ok I will investigate
  2. That is an intentional change in this PR. Favorites are now ordered alphabetically and can only be moved between folders.

@huboneo huboneo requested a review from oskarhane September 11, 2019 12:19
@huboneo
Copy link
Contributor Author

huboneo commented Sep 11, 2019

@oskarhane I've updated the PR to address 1. I also found two unrelated bugs from the browser-lambda-parser PR which I fixed. Please check my changes to build_scripts so that I didn't miss anything

Copy link
Member

@oskarhane oskarhane left a comment

Choose a reason for hiding this comment

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

Great!

Dragging a .png file shows error message in top banner rather than in a new error frame. Is that intentional?

@@ -98,7 +98,7 @@ module.exports = [
use: ['style-loader', 'css-loader']
},
{
test: /\.svg$/,
test: /\.(svg|png)/,
Copy link
Member

Choose a reason for hiding this comment

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

This is still an issue.
oskarhane-mbpt 2019-09-12 at 08 49 45

@@ -23,7 +23,11 @@ const path = require('path')
module.exports = [
{
test: /\.(js|jsx)$/,
exclude: /(node_modules)|(cypher-codemirror)|(test_utils)|(dist)/,
include: [
Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit worried that this might break something but from my tests it all seems fine.
We should verify each step in the build pipeline after this is merged.

@huboneo
Copy link
Contributor Author

huboneo commented Sep 12, 2019

@oskarhane Sorry completely forgot about the webpack bug. Fixed:
Screenshot 2019-09-12 at 09 25 51

Copy link
Member

@oskarhane oskarhane left a comment

Choose a reason for hiding this comment

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

LGTM 🎉

Ignore the E2E test failures and merge.

@huboneo huboneo merged commit 63e6490 into neo4j:master Sep 12, 2019
@huboneo huboneo deleted the feature-export-favorites branch September 12, 2019 11:37
@huboneo huboneo mentioned this pull request Sep 13, 2019
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.

2 participants