Compass 528 reactify home #675
Conversation
satyasinha
force-pushed
the
COMPASS-528-Reactify-Home
branch
from
b284617
to
89cbce4
Compare
|
I am currently unsure at what level the instance fetching is meant to take place once app/home/index.js goes away. Is it more upstream towards src/index.js or downstream within the react home components? https://github.com/10gen/compass/pull/675/files#diff-1bfc26616a1850249d97ae8f9c32568eR14 |
satyasinha
force-pushed
the
COMPASS-528-Reactify-Home
branch
2 times, most recently
from
f03bb6f
to
c124487
Compare
|
This is a fairly big change it would be good if it was reviewed by more than 1 person. |
| #### Definitions | ||
|
|
||
| | Key | Description | | ||
| |-----------------------------------|-----------------------------------------------| |
There was a problem hiding this comment.
Could we have a go at populating the rest of this README?
| # Compass Home Package | ||
|
|
||
| Home layout for Compass view that switches between Server Stats, Collections | ||
| Table and Collection views. |
There was a problem hiding this comment.
Perhaps?
switches between different views based on the current namespace:
* instance-level (Databases / Performance, empty string namespace)
* database-level (navigate and manage collections within a database)
* collection-level (Documents, Indexes, Schema, Explain Plan, Validation)
| @@ -0,0 +1,78 @@ | |||
| const app = require('ampersand-app'); | |||
| const Reflux = require('reflux'); | |||
| // const toNS = require('mongodb-ns'); | |||
There was a problem hiding this comment.
Remove as it's on line 7 too?
|
|
||
| class ConnectedHome extends React.Component { | ||
| /** | ||
| * Connect <Validation /> component to store and render. |
| @@ -29,6 +29,8 @@ const SidebarStore = Reflux.createStore({ | |||
| */ | |||
| init() { | |||
| NamespaceStore.listen(this.onNamespaceChanged.bind(this)); | |||
| this.listenToExternalStore('App.InstanceStore', | |||
| this.onInstanceRefreshed.bind(this)); | |||
There was a problem hiding this comment.
This isn't broken over multiple lines anywhere else in Compass?
There was a problem hiding this comment.
Also:
onInstanceRefreshed looks confusingly close to
onInstanceFetched, is there any obviously more distinct name for either of these, or is there more refactoring that could be done to make it clearer if they are the same thing?
There was a problem hiding this comment.
good call, I found that if we're listening to the instance store we no longer need a separate setInstance method.
| const StoreConnector = app.appRegistry.getComponent('App.StoreConnector'); | ||
|
|
||
| const Home = require('./home'); | ||
| const Store = require('../store'); |
satyasinha
force-pushed
the
COMPASS-528-Reactify-Home
branch
from
c124487
to
7f64c1f
Compare
| } | ||
|
|
||
| Home.propTypes = { | ||
| hasContent: React.PropTypes.bool, |
There was a problem hiding this comment.
Is the hasContent prop used at all?
| const Home = require('./home'); | ||
| const HomeStore = require('../store'); | ||
|
|
||
| // const debug = require('debug')('mongodb-compass:validation:index'); |
There was a problem hiding this comment.
Even though it's commented out, can you make that mongodb-compass:home:index?
| @@ -227,6 +227,28 @@ var Application = View.extend({ | |||
| var StatusAction = app.appRegistry.getAction('Status.Actions'); | |||
| StatusAction.setMessage(err); | |||
| }, | |||
| onInstanceFetched: function() { | |||
| // TODO: Remove this line | |||
There was a problem hiding this comment.
when should this happen? are we blocked on it currently?
There was a problem hiding this comment.
We are not blocked on it for this PR.
It should happen when connect dialog is reactified. At the moment there is an instance-store that handles most of the instance related events, except for this one. Ideally this initial fetch should all happen on the store but since stores are pulled in before connections and routes have been set it won't work at the moment (At least that's been my experience).
We can fix this at a later date, when connect and app/index.js are reactified.
There was a problem hiding this comment.
@rueckstiess It works and is not a blocker to this going through. It's more that it remains and appears to be a quick hack rather than a proper fix and we couldn't resolve it easily when we tried for around half an hour on Wednesday. I have created https://jira.mongodb.org/browse/COMPASS-563 to track it.
satyasinha
force-pushed
the
COMPASS-528-Reactify-Home
branch
3 times, most recently
from
81f1163
to
325eed8
Compare
satyasinha
force-pushed
the
COMPASS-528-Reactify-Home
branch
from
325eed8
to
bb44464
Compare
|
Overall LGTM. 👍 Remaining TODO that was present before this ticket was attempted is tracked as COMPASS-563. Let's get this out onto master as we're pretty sure it's working correctly and it's always a good thing to say goodbye to more of this Jade/Ampersand stuff. |
No description provided.