Skip to content
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

Events Widget Component #557

Merged
merged 14 commits into from
Apr 24, 2019
Merged

Events Widget Component #557

merged 14 commits into from
Apr 24, 2019

Conversation

bafonins
Copy link
Contributor

Summary:

This PR adds the <Events.Widget /> component to the console as well as some groundwork for the entity events pages.
References #28 and #553

Screenshot 2019-04-22 at 16 36 00

Screenshot 2019-04-22 at 16 42 53

Changes:

  • Add the <Events /> (currently as a stub) and <Events.Widget /> components.
  • Add the <List /> component
  • Add the <Event /> component of different types
  • Add the <Status /> component
  • Add entity id selectors

cc @htdvisser

@bafonins bafonins added the c/console This is related to the Console label Apr 22, 2019
@bafonins bafonins added this to the April 2019 milestone Apr 22, 2019
@bafonins bafonins requested a review from kschiffer April 22, 2019 14:40
@bafonins bafonins self-assigned this Apr 22, 2019
@htdvisser
Copy link
Contributor

In the "Latest Data" you show events for both admin-gtw and admin-app. Gateway and Application IDs do not share the same namespace, so it's possible that there would be a Gateway with the same ID as an Application. Maybe we should indicate the entity type? I can imagine we already have icons for the navigation, perhaps re-use those and put them in front of the string IDs?

How are events rendered if they contain more than one identifier? For example, if an OAuth token exchange event contains the identifiers of both the User ID and the Client ID.

@bafonins
Copy link
Contributor Author

In the "Latest Data" you show events for both admin-gtw and admin-app.

This is only for presentation purposes and wont be the case in the console. For this PR events are hardcoded.

How are events rendered if they contain more than one identifier?

The first matching id will be shown, see https://github.com/TheThingsNetwork/lorawan-stack/blob/73b10e22e05aff82749d3a392949f606b0fc4b55/pkg/webui/lib/selectors/id.js#L49-L58

@bafonins bafonins changed the title Feature/28 webui events widget Event Widget Component Apr 23, 2019
@bafonins bafonins changed the title Event Widget Component Events Widget Component Apr 23, 2019
@bafonins bafonins force-pushed the feature/28-webui-events-widget branch from 73b10e2 to 8abebd7 Compare April 24, 2019 03:55
@bafonins bafonins requested review from kschiffer and removed request for kschiffer April 24, 2019 03:55
display: flex
align-items: center
flex-wrap: nowrap
font-family: 'IBM Plex Mono', Consolas, Monaco, 'Andale Mono', 'Ubuntu Mono', monospace
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
font-family: 'IBM Plex Mono', Consolas, Monaco, 'Andale Mono', 'Ubuntu Mono', monospace
font-family: $font-family-mono

import style from './widget.styl'

const m = defineMessages({
lastActivity: 'Last network activity',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
lastActivity: 'Last network activity',
lastActivity: 'Latest events',

Although it says so in the screen design, network activity is misleading when we also show crud events.

align-items: center
flex-wrap: nowrap
font-family: 'IBM Plex Mono', Consolas, Monaco, 'Andale Mono', 'Ubuntu Mono', monospace

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
font-size: $fs.s


&-icon
margin-right: $cs.xs
nudge('up', 2px)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
nudge('up', 2px)
font-size: 1.1rem
nudge('up', 1px)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also make sure that icons are $c-icon-fill if no other color is set.

size="small"
items={events}
renderItem={this.renderEvent}
emptyMessage={m.noEvents}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The empty message needs a special styling to be $tc-subtle-gray

/>
<Link to={toAllUrl}>
<Message content={m.seeAllActivity} />
<Icon icon="arrow_forward" />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<Icon icon="arrow_forward" />

Use the character rather.

// limitations under the License.

.widget

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

redundant new line

background-color: $c-active-blue

&:after
animation: goodPulse 2s infinite
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The animation is actually thought to pulse whenever an event arrives. It can then also be a little more noticeable. Obviously not MVP critical, so don't do this.

const m = defineMessages({
lastActivity: 'Last network activity',
seeAllActivity: 'See all activity',
noEvents: 'No events emitted by {entityId}',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
noEvents: 'No events emitted by {entityId}',
noEvents: '{entityId} has not sent any events recently',

value={time}
date={false}
/>
<span className={style.eventEmitter}>{emitter}</span>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Think about size limiting these. text-overflow: ellipsis

@bafonins bafonins force-pushed the feature/28-webui-events-widget branch from 8abebd7 to 01128d1 Compare April 24, 2019 12:49
@bafonins bafonins requested a review from kschiffer April 24, 2019 12:49

import Link from '../../link'
import Message from '../../../lib/components/message'
import Icon from '../../icon'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove

Suggested change
import Icon from '../../icon'

</Link>
</div>
<List
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Atm, the list will keep growing when list items overflow. It should rather have a fixed height and use overflow: hidden (and possibly also limit the number of items rendered, accordingly).

@bafonins bafonins force-pushed the feature/28-webui-events-widget branch 2 times, most recently from 4e951bc to 8d3730b Compare April 24, 2019 14:46
@bafonins bafonins force-pushed the feature/28-webui-events-widget branch from 8d3730b to 97885ff Compare April 24, 2019 15:03
@bafonins bafonins merged commit df50617 into master Apr 24, 2019
@bafonins bafonins deleted the feature/28-webui-events-widget branch April 24, 2019 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c/console This is related to the Console
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants