-
Notifications
You must be signed in to change notification settings - Fork 310
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
Single Device Overview Page (stubbed) #97
Single Device Overview Page (stubbed) #97
Conversation
Remove duplicate api entries
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 good, just small remarks
hardwareVersion: 'Hardware version', | ||
firmwareVersion: 'Firmware Version', | ||
activationInfo: 'Activation Info', | ||
rootKeyId: 'Root Key Id', |
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.
we have Device ID
, i guess it should be Root Key ID
activationInfo: 'Activation Info', | ||
rootKeyId: 'Root Key Id', | ||
sessionInfo: 'Session Info', | ||
fwdNtwkKey: 'FNwkSIntKey', |
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.
beyond the scope of this PR, however worth noting. we should come up with a component to show the full name of such fields. something like a tooltip with the question mark icon should do the job
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.
Yes, absolutely agree. Was briefly considering adding this, but not important enough for now.
{ title: sharedMessages.overview, name: 'overview' }, | ||
{ title: sharedMessages.data, name: 'data' }, | ||
{ title: sharedMessages.location, name: 'location' }, | ||
{ title: sharedMessages.payloadFormatter, name: 'develop' }, |
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.
If we use Payload Formatter
here, it seems that it should be changed for the side navigation as well, since atm it is Payload Formats
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.
Ah, no actually Payload Formats
is correct.
</Col> | ||
<Col md={12} lg={6}> | ||
<div className={style.activityPlaceholder}> | ||
<h4>Latest Data</h4> |
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.
<h4>Latest Data</h4> | |
<Message content={sharedMessages.latestData} component="h4" /> |
the latestData
entry should be added to the shared messages
<div>Activity Panel Placeholder</div> | ||
</div> | ||
<div className={style.locationPlaceholder}> | ||
<h4>Location</h4> |
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.
<h4>Location</h4> | |
<Message content={sharedMessages.location} component="h4" /> |
return ( | ||
<Container> | ||
<IntlHelmet> | ||
<title>{device.name || device_id}</title> |
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.
shall we just stick to ids?
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.
With the ID being referenced clearly in the "General Info" bit, I think it's ok to use this approach here. Otherwise, there is almost no need to have a name
field at all.
|
||
@connect(function ({ device, application }, props) { | ||
return { | ||
appName: application.application.name, |
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 understand the whole name-id relation for devices is not yet known but for applications it might be not preset
} | ||
}) | ||
@withBreadcrumb('device.single', function (props) { | ||
const { appId, devId } = 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.
there is not appId
directly accessible from the props. you should get it from props.match.params.appId
const { appId, devId } = props | ||
return ( | ||
<Breadcrumb | ||
path={`/console/applications/${appId}/devices/${devId}`} |
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.
breadcrumb link is broken, see above
Fix IntlHelmet crashes
1ebb42f
to
8a98bc5
Compare
Summary:
This PR adds the overview page for devices, using api stubs, as well as placeholders for the "Latest Activity" and "Location" part. See also #25.
Since the PR uses stubs, I don't want to merge it to master but rather use the opportunity to get a review, before merging it into
feature/webui-single-device
, which I will open another PR for the stub replacement on, before merging it to master.Changes:
faker
data made persistent so that it stays between refreshsreact-helmet
template and title for devicesNotes for Reviewers:
Distinction between
name
andid
of a device is a little hard to make out with the fake data, as it is completely different. The name is used in the page title at the top, the id is used in the first cell of the overview. The connection should be clearer with real data. We need to make the distinction clearer also in our tables and overview pages of other entities.Release Notes: (optional)