Aerogear 7706 Add base build view to mobile client page #27
Conversation
StephenCoady
changed the title
|
In my browser, when I click the menu button, the menu is not displayed properly. Otherwise LGTM.
|
| </React.Fragment> | ||
| ); | ||
|
|
||
| const buildConfig = { |
There was a problem hiding this comment.
Only a question here, is there a benefit to defining a const here over a function defined in the component itself returning the jsx?
There was a problem hiding this comment.
This is just the way Jan mocked the data for now. Once we have the backend hooked up this will be gone completely and props will be passed to this component.
There was a problem hiding this comment.
Typical the one I clicked on to leave the comment, I meant to call out either const actions or const heading, is there any benefit to having them here?
ui/src/components/Client.js
Outdated
| @@ -19,7 +92,7 @@ class Client extends Component { | |||
| <ConfigurationView /> | |||
| </TabPane> | |||
| <TabPane eventKey={2}> | |||
| Builds | |||
| <MobileClientBuildOverviewList mobileClientBuilds={mobileClientBuilds}></MobileClientBuildOverviewList> | |||
There was a problem hiding this comment.
MobileClientBuildsList as a name?
What this list displays is a list of buildConfigs which can be got from /api/buildconfigs. Can you try using the endpoint instead of mock since it has been merged now?
| @@ -0,0 +1,37 @@ | |||
| import React, { Component } from 'react'; | |||
There was a problem hiding this comment.
Aware this isn't your component but maybe BuildConfigDetails as a name?
|
|
||
|
|
||
| const actions = () => ( | ||
| <React.Fragment id="mobile-client-actions" pullRight> |
|
|
||
| return ( | ||
| <MobileListViewItem | ||
| className="overview-list-view-item" |
|
@StephenCoady A few things visually. The header size is different to the overview component as that header contains more elements/information. I think it might make sense to set a height on the header to keep them the same. It feels jarring having inconsistent components. Also can you take a look at this component in the community release version and match the font sizing etc/icon styles etc. |
| <div className="build-config"> | ||
| <Row> | ||
| <Col md={6}> | ||
| <b>Repo Url</b> |
There was a problem hiding this comment.
Can you add semi colons on the <b>s?
StephenCoady
force-pushed
the
AEROGEAR-7706
branch
2 times, most recently
from
2a3b942
to
e16b81b
Compare
StephenCoady
force-pushed
the
AEROGEAR-7706
branch
from
e16b81b
to
c33865f
Compare
|
@sedroche addressed your comments. |
Changes