User profile pages (favorites, created content, recent activity, security & access) #1615
Conversation
mistercrunch
commented
Nov 16, 2016
mistercrunch
commented
|
|
mistercrunch
changed the title
| <h1>{dashboard.dashboard_title}</h1> | ||
| <h1> | ||
| {dashboard.dashboard_title} | ||
| <span is class="favstar" class_name="Dashboard" obj_id={dashboard.id} /> |
There was a problem hiding this comment.
should this be <span className="favstar"? typo maybe?
There was a problem hiding this comment.
ReactDOM's is flag is a way to allow for passing raw unstandard html attributes which in this case I needed if I didn't want to alter the way the "favstar" thing was designed. Once that flag is on everything is like raw html, so class instead of className.
We need to redesign FavStar as a react component, in the meantime it's a little hack to make it work as is.
| user: React.PropTypes.object.isRequired, | ||
| }; | ||
|
|
||
| class App extends React.PureComponent { |
There was a problem hiding this comment.
this could be a stateless functional component.
export default function App(props) {
return (
<div>...</div>
);
}
| }; | ||
| } | ||
|
|
||
| imgLoading() { |
There was a problem hiding this comment.
looks like we aren't using this anywhere, let's delete it.
| import React from 'react'; | ||
| import moment from 'moment'; | ||
| import TableLoader from './TableLoader'; | ||
|
|
There was a problem hiding this comment.
oh interesting, the linter didn't trigger because it's unaware of what's hapenning inside the backticks
| const propTypes = { | ||
| user: React.PropTypes.object.isRequired, | ||
| }; | ||
| const Security = ({ user }) => ( |
There was a problem hiding this comment.
let's use named functions for SFC's
export default function Security({ user }) {
return (
...
);
}
| import TableLoader from './TableLoader'; | ||
| import moment from 'moment'; | ||
| import $ from 'jquery'; | ||
|
|
|
|
||
| const propTypes = { | ||
| dataEndpoint: React.PropTypes.string.isRequired, | ||
| loadingNode: React.PropTypes.node, |
There was a problem hiding this comment.
do we need to expose a custom loading node for this component. i can see how we could simplify this if we hard code the loading node in this component.
| delete tableProps.dataEndpoint; | ||
| delete tableProps.columns; | ||
| if (this.state.isLoading) { | ||
| return this.props.loadingNode; |
There was a problem hiding this comment.
i'm surprised the linter doesn't complain about multiple return statements. would be nice to follow the convention of a single return statement for the render function.
| @@ -0,0 +1,13 @@ | |||
| .tab-pane { | |||
| min-height: 400px; | |||
| border-left: 1px solid #bbb; | |||
There was a problem hiding this comment.
could simplify this to:
border: 1px solid #bbb;
border-top: 0px;
| @@ -48,7 +49,7 @@ const config = { | |||
| }, | |||
| { | |||
| test: /\.jsx?$/, | |||
| exclude: APP_DIR + '/node_modules', | |||
| exclude: /(node_modules|bower_components)/, | |||
There was a problem hiding this comment.
why do we need this change? where do we use bower_components?
There was a problem hiding this comment.
I think I hit some problems I may have ended fixing in other ways, going to try to roll this back
| <div class="navbar-header"> | ||
| <button type="button" class="navbar-toggle" data-toggle="collapse" data-target=".navbar-collapse"> | ||
| <span class="icon-bar"></span> | ||
| <span class="icon-bar"></span> | ||
| <span class="icon-bar"></span> | ||
| </button> | ||
| <a class="navbar-brand" href="{{appbuilder.get_url_for_index}}"> | ||
| <a class="navbar-brand" href="/superset/profile/{{ current_user.username }}/"> |
|
some comments, but overall this is DOPE! nice! |
mistercrunch
force-pushed
the
user_profile
branch
from
dca331a to
96c20fc
Compare
|
Alright, all comments addressed except for the multiple Thanks for the review! |
mistercrunch
force-pushed
the
user_profile
branch
from
96c20fc to
7ef71ac
Compare
mistercrunch
added
🏷️ bot
