-
Notifications
You must be signed in to change notification settings - Fork 46
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
PXP-2464 fix/homepage chart REST API #476
Conversation
Branch deployed in https://qingya.planx-pla.net/, now its index page is open to public. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me. Nice work!
fb6f717
to
9893651
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good! just had one comment in the code.
Also I see typeof x !== 'undefined'
in the code a lot, you could shorten it to !!x
if you just want to check if a value exists... it would also capture null
and false
src/Login/Login.jsx
Outdated
@@ -68,7 +78,7 @@ class Login extends React.Component { | |||
<hr className='login-page__separator' /> | |||
<div className='body-typo'>{this.props.data.text}</div> | |||
{ | |||
this.props.providers.map( | |||
this.props.providers && this.props.providers.map( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this necessary to add if you added default props?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I changed the code yesterday and forgot to commit... let me update that! thanks
import './page.less'; | ||
|
||
class IndexPageComponent extends React.Component { | ||
constructor(props) { | ||
super(props); | ||
getProjectsList(); | ||
getProjectNodeCounts((res) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that this should go in componentDidMount
, not the constructor.
https://reactjs.org/docs/react-component.html#constructor
This PR change homepage chart to hit REST API instead of using relayer, and also enable
public
option for index page.components.index.homepageChartNodes
for required nodes and chart labels.components.index.public
.gitops.json
/${APP}.json
towards using REST API, then relayer-related code could be moved.New Features
components.index.public: true
.Breaking Changes
components.index.homepageChartNodes
for required nodes and chart labels.