Issue #309 - Browse Page #375

storrisi · 2018-11-24T17:06:13Z

Fixes #309

Initial WIP PR with some refactoring of the AbstractPageDefinitions / PageDefinitions, in order to clean the code, remove unused method, move shared methods into utils classes or HoC.

Most of the methods should be used by the new PageBrowse , so the refactor it's necessary to avoid code duplication.

jeffmcaffer

Overall a great direction. I've left some comments in the code. Some general things here.

Need to distinguish strategies for libaries/utils, hierarchies, components, and HOCs. Some things here I would have expected to be one are another (see file comments). I generally prefer composition over class hierarchy but certainly room for both.
With the intro of Browse we now have user-managed and system-managed lists. Definitions is a list created and managed by the user. It should have share, DnD, ... Browse OTOH is filtered and sorted by they user but list itself is driven by the system. (likely does not have share, DnD, ...). It would be interesting to think of the user behaviors in each of these contextx.

jeffmcaffer · 2018-11-24T17:09:21Z

src/components/Navigation/Pages/PageBrowse.js

+}
+
+function mapDispatchToProps(dispatch) {
+  return bindActionCreators(


As a general formatting comment please go wide rather than tall. Here all on one line please. Similar comment throughout the code.

That's a Prettier default behaviour, i will check if we can avoid it

If you just don;t put any line breaks in then Prettier will format it like this

function mapDispatchToProps(dispatch) { return bindActionCreators({ uiNavigation }, dispatch) }

jeffmcaffer · 2018-11-24T17:46:36Z

src/components/Navigation/Pages/PageBrowse.js

@@ -0,0 +1,36 @@
+import React from 'react'


copyright/license header

jeffmcaffer · 2018-11-24T17:53:44Z

src/components/PageDefinitions.js

@@ -224,8 +148,49 @@ export class PageDefinitions extends AbstractPageDefinitions {
    )
  }

-  updateList(o) {
-    return uiBrowseUpdateList(o)
+  renderFilter(list, title, id) {


feels like this should be factored out as well like SortList

I agree, i've moved only some blocks of code for now, this should become a separate component

jeffmcaffer · 2018-11-24T18:00:09Z

src/components/PageDefinitions.js

+    const { sequence, showFullDetail, path, currentComponent, currentDefinition } = this.state
+    return (
+      <Grid className="main-container">
+        <ContributePrompt


probably should put all the popups at the top or bottom. I like having explicit render methods for them. Nice

jeffmcaffer · 2018-11-24T18:08:20Z

src/components/AbstractPageDefinitions.js

+    })
+  }
+
+  doSave() {


will be interesting to see how widely usable the share functions will be. Browse likely should not allow sharing (at least not until we do mulitselect) as the list in Browse is "infinite".

jeffmcaffer · 2018-11-24T18:11:51Z

src/components/AbstractPageDefinitions.js

+    dispatch(uiBrowseUpdateList({ add: component }))
+  }
+
+  dropZone(child) {


Can this be part of a Drop component? along with onDrop above?

Yes, for sure

jeffmcaffer · 2018-11-24T18:12:40Z

src/utils/drop.js

+ * Abstract methods for Dropping functionality
+ *
+ */
+export default class Drop {


Should this be a Component? Where else would we use this function?

I was starting building a utils class thinking of reusing all the related methods, but your comment pointed me out in the right direction.
We don't need a class of methods for the Drop functionality, but it should be a real Component, plugged in like the DropZone library

jeffmcaffer · 2018-11-24T18:15:11Z

src/components/AbstractPageDefinitions.js

+    }
+  }
+
+  onAddComponent(value) {


this will not be generally usable. Only the definitions list view (and harvest) are lists that are user managed.

jeffmcaffer · 2018-11-24T18:18:51Z

src/components/AbstractPageDefinitions.js

+    return { coordinates }
+  }
+
+  loadFromListSpec(list) {


this (and related methods above) are only relevant to user-managed lists (definitions and harvest).

storrisi · 2018-11-24T23:39:01Z

Thanks @jeffmcaffer , the difference between user-managed and system-managed is interesting.
I will work in order to distinguish the two behaviour, probably creating some HoC or Abstract Components that contains the logic.
What do you think?

jeffmcaffer · 2018-11-25T05:15:54Z

User vs system may be useful. I was just observing the difference as I read through your changes. Not sure if that is the major pivot or just a characteristic. From a code point of view if we use composition more then we don't have to think too hard about that as a hierarchy as things are a bit more mix and match.

From a user experience point of view it may be useful to essentially capture user expectations (e.g., as a user I am constructing/managing a list vs observing an existing list).

…folders * changed AbstractPageDefinitions to UserManagedList

* Updated code documentation * Added new ProviderButtons component

jeffmcaffer

First pass of comments. Generally I like the direction but but am unsure of some of the finer points of how/why bits are being factored out and where they are being put.

jeffmcaffer · 2018-11-29T05:51:25Z

src/components/Navigation/Pages/PageBrowse/PageBrowse.js

+import { ROUTE_BROWSE } from '../../../../utils/routingConstants'
+import UserManagedList from '../../../UserManagedList'
+
+class PageBrowse extends UserManagedList {


Browse should be a System managed list. The list is generated by the system. all the user can do is sort/filter. They can't actually add or remove things from the list nor should they be able to share or save. (perhaps they can share/save but that would just be their sort/filtering settings)

src/components/Navigation/Pages/PageContribution/ButtonsBar.js

jeffmcaffer · 2018-11-29T05:57:38Z

src/components/Navigation/Pages/PageContribution/PageContribution.js

+  }
+
+  render() {
+    const { components, definitions } = this.props


Seems like this used to be shared. Did we lose some shared code that used to be in AbstractPageDefinitions

jeffmcaffer · 2018-11-29T05:58:41Z

src/components/Navigation/Pages/PageDefinitions/ButtonsBar.js

+import ButtonWithTooltip from '../../Ui/ButtonWithTooltip'
+import ShareButton from '../../Ui/ShareButton'
+
+export default class ButtonsBar extends Component {


similar comment to previous. is this reusable or specific to PageDefintions?

jeffmcaffer · 2018-11-29T06:01:27Z

src/components/Navigation/Sections/ButtonsBar.js

+import ButtonWithTooltip from '../Ui/ButtonWithTooltip'
+import ShareButton from '../Ui/ShareButton'
+
+export default class ButtonsBar extends Component {


duplicate with the other ButtonsBar above?

jeffmcaffer · 2018-11-29T06:04:26Z

src/components/Navigation/Sections/FilterBar.js

@@ -0,0 +1,55 @@
+import React, { Component } from 'react'


wrt folder naming, how are you deciding what is "navigation"? There are some places where there is a "ui" segment in the path? This is all UI so that designation is unclear. Here what is the significance of "sections"?

More generally it feels like we are making a complex and deep folder structure where in reality we just have a bunch of components that get assembled in different ways to form the UI. There are a few clear things that go together but otherwise, most things should be generic and stand-alone.

jeffmcaffer · 2018-11-29T06:08:13Z

src/components/Navigation/Sections/ButtonsBar.js

+    shareGist: PropTypes.func
+  }
+
+  onSelect = type => {


Can this be part of ShareButton?

jeffmcaffer · 2018-11-29T06:08:45Z

src/components/Navigation/Ui/SortList.js

+import { DropdownButton, MenuItem } from 'react-bootstrap'
+import PropTypes from 'prop-types'
+
+class SortList extends Component {


SortButton?

jeffmcaffer · 2018-11-29T06:19:18Z

src/components/Navigation/Pages/PageContribution/PageContribution.js

+import FilterBar from '../../Sections/FilterBar'
+import FullDetailPage from '../../../FullDetailView/FullDetailPage'
+
+class PageContribution extends UserManagedList {


Actually I wonder if this should be SystemManaged? If it is the list of definitions in a curation, that is not something that the user can manage (given the PR is already opened). In the future you could imagine that we'll allow uses to "edit" the curation but there are a number of workflow issues with that so ti will be a while.

jeffmcaffer · 2018-11-29T06:23:40Z

src/components/Navigation/Pages/PageContribution/ButtonsBar.js

+        </Button>
+        &nbsp;
+        <Button bsStyle="success" disabled={hasChanges} onClick={doSave}>
+          Save


what does "save" do? We have "share" but not sure about save.

* SavePopUp factored out * Fixed readOnly prop in GlobsPicker

storrisi · 2018-11-30T15:10:04Z

@jeffmcaffer i've finalized the work here.
This is how the page looks like:

As you can see this page is quite a "mix" of a System Managed list , which is basically Read-Only, and a User Managed List.
This kind of page has all the System Managed List functionalities, but allow the user to edit the definitions and make a curation.
Beside that, the user can't modify the returned list, neither share it.

Two small advice:

When a user selects a provider, the application calls a "/browse?pattern={provider}" API, which is not already implemented
given that, i've hidden the "Browse" voice from the Navigation Menu.
You can decide to wait to merge this PR until we have completed the service implementation, or merge it anyway and keep the page hidden.

…e into webyourmind/issue-309

jeffmcaffer

Just about there. Some more minor comments added.

Note, looking through the commits, there is f02ec18 which seems to remove EntitySpec.fromString() but that change is not showing up in the overall PR. I do not think we should remove that. It should, perhaps call fromPath. that would be a good mirroring of the toPath and toString functionality.

src/actions/definitionActions.js

src/components/FullDetailView/FullDetailPage.js

src/components/Navigation/Pages/PageContribution/ButtonsBar.js

src/components/UserManagedList.js

src/utils/utils.js

jeffmcaffer · 2018-11-30T17:49:04Z

src/reducers/uiReducer.js

@@ -52,17 +52,11 @@ const initialStateNavigation = [
    isSelected: false
  },
  /*{


hmmm, not sure. there are some "hidden" routes that we use for deep linking to both the full details PAGE and the page that shows all of a particular PR. Not sure which if any of these is "inspect".

Also, will you need to uncomment this to have Browse show up in the UI?

Yes, this is a temporary workaround if you want to merge this PR and don't allow anyone digging into this page while there isn't any related API available.

storrisi · 2018-12-03T14:01:13Z

I've updated the PR based on your suggestions and also changed the name of the api call method in order to be more generic.

storrisi added 6 commits November 23, 2018 16:10

Added new Browse route and Page

74564ce

Initial refactor of PageDefinition

8b259db

SortList refactoring

5a278fd

Moved all renderers into PageDefinition

860e941

Removed unused methods

126e1dc

Drop utils

2a3c032

storrisi requested a review from jeffmcaffer November 24, 2018 17:06

jeffmcaffer reviewed Nov 24, 2018

View reviewed changes

storrisi added 7 commits November 26, 2018 11:58

Moved all drop logics into a Drop component

3cccdc0

FIlterList component

96c8566

Moved all buttons to specific components

4f20acb

* Moved PageDefinitions and PageContributions to specific components …

e3ea53a

…folders * changed AbstractPageDefinitions to UserManagedList

Update DropComponent PropTypes

93d19a4

Separating methods from SystemManagedList and UserManagedList

ef0b8cf

* PageBrowse structure extending SystemManagedList

fa7b874

* Updated code documentation * Added new ProviderButtons component

jeffmcaffer reviewed Nov 29, 2018

View reviewed changes

storrisi and others added 8 commits November 29, 2018 20:01

Browse Page methods to retrieve data

e44910b

* added Browse Page curate functionality

b3e640e

* SavePopUp factored out * Fixed readOnly prop in GlobsPicker

Added popularity sorting

423db46

Defined specif server API

7c5608f

Commented Browse menu element since API is not ready

7468787

Fixed testing

a9bbcea

Added tests

767f880

Merge branch 'master' into webyourmind/issue-309

38336a2

storrisi changed the title ~~WIP : Issue #309 - Browse Page~~ Issue #309 - Browse Page Nov 30, 2018

storrisi added 2 commits November 30, 2018 16:22

Removed unused methods

f02ec18

Merge branch 'webyourmind/issue-309' of github.com:WebYourMind/websit…

a12ee77

…e into webyourmind/issue-309

jeffmcaffer reviewed Nov 30, 2018

View reviewed changes

storrisi and others added 3 commits December 3, 2018 14:57

Updated actions methods using async/await

28c73d1

Merge branch 'master' into webyourmind/issue-309

d3af8ae

Removed warnings

876a64c

jeffmcaffer merged commit 2116a32 into clearlydefined:master Dec 3, 2018

This was referenced Dec 3, 2018

Issue #364 - dangling tooltips #383

Merged

Issue #339 - Show suggested data #373

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #309 - Browse Page #375

Issue #309 - Browse Page #375

storrisi commented Nov 24, 2018

jeffmcaffer left a comment

jeffmcaffer Nov 24, 2018

storrisi Nov 24, 2018

jeffmcaffer Nov 25, 2018

jeffmcaffer Nov 24, 2018

jeffmcaffer Nov 24, 2018

storrisi Nov 24, 2018

jeffmcaffer Nov 24, 2018

jeffmcaffer Nov 24, 2018

jeffmcaffer Nov 24, 2018

storrisi Nov 24, 2018

jeffmcaffer Nov 24, 2018

storrisi Nov 24, 2018

jeffmcaffer Nov 24, 2018

jeffmcaffer Nov 24, 2018

storrisi commented Nov 24, 2018

jeffmcaffer commented Nov 25, 2018

jeffmcaffer left a comment

jeffmcaffer Nov 29, 2018

jeffmcaffer Nov 29, 2018

jeffmcaffer Nov 29, 2018

jeffmcaffer Nov 29, 2018

jeffmcaffer Nov 29, 2018

jeffmcaffer Nov 29, 2018

jeffmcaffer Nov 29, 2018

jeffmcaffer Nov 29, 2018

jeffmcaffer Nov 29, 2018

storrisi commented Nov 30, 2018

jeffmcaffer left a comment

jeffmcaffer Nov 30, 2018

storrisi Dec 1, 2018

storrisi commented Dec 3, 2018

Issue #309 - Browse Page #375

Issue #309 - Browse Page #375

Conversation

storrisi commented Nov 24, 2018

jeffmcaffer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

storrisi commented Nov 24, 2018

jeffmcaffer commented Nov 25, 2018

jeffmcaffer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

storrisi commented Nov 30, 2018

jeffmcaffer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

storrisi commented Dec 3, 2018