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

Activity Panel: UI #687

Merged
merged 5 commits into from Apr 14, 2019

Conversation

Projects
None yet
3 participants
@bpierre
Copy link
Member

bpierre commented Apr 10, 2019

Base PR: #674

@bpierre bpierre requested review from 2color and sohkai Apr 10, 2019

@bpierre bpierre changed the title Fix the activity button Activity Panel: UI Apr 11, 2019


const _onBlur = e => {
const handleBlur = e => {
let _handler
const currentTarget = e.currentTarget
setTimeout(() => {

This comment has been minimized.

Copy link
@2color

2color Apr 11, 2019

Contributor

@bpierre Do you know what the timeout is for?

This comment has been minimized.

Copy link
@bpierre

bpierre Apr 11, 2019

Author Member

It is to prevent a click on the toggle button, while the panel is opened, to close it (because of the blur) and immediately reopening it (because of the click). But I had some weird behaviors that I suspect are caused by it, so I want to try a few alternatives to see if it fixes the issue.

}

const StatusMessage = ({ activity }) => {
const [icon, content] = getStatusData(activity)

This comment has been minimized.

Copy link
@2color

2color Apr 11, 2019

Contributor

It's interesting how React hooks made it a thing to return 2 value array tuples :)

Actually, it might be Object.entries()

This comment has been minimized.

Copy link
@bpierre

bpierre Apr 11, 2019

Author Member

Yes totally, it’s always interesting to see patterns being upgraded from “controversial” to normal, once they reach mainstream usage and acceptation :-)

const [activitiesReady, setActivitiesReady] = React.useState({})

const activityItems = useMemo(
() => activities.sort((a, b) => b.createdAt - a.createdAt),
[activities, getAppByProxyAddress]

This comment has been minimized.

Copy link
@2color

2color Apr 11, 2019

Contributor
Suggested change
[activities, getAppByProxyAddress]
[activities]

Shouldn't merely activities suffice?

)
}

getAppByProxyAddress = proxyAddress => {

This comment has been minimized.

Copy link
@2color

2color Apr 11, 2019

Contributor

What do you think about pulling this out and making a curried version that can be imported anywhere, i.e. getAppByProxyAddress = apps => proxyAddress => {...

This should allow us to reduce the size of wrapper.

This comment has been minimized.

Copy link
@bpierre

bpierre Apr 11, 2019

Author Member

Yes let’s do this, Wrapper is large enough already! Adding this also made me think that it could be nice to move the loaded apps in a React context.

@2color

This comment has been minimized.

Copy link
Contributor

2color commented Apr 11, 2019

Starting to look really nice!

One thing I noticed was the pending and failed icons are a little bit higher than they should be:
Screen Shot 2019-04-11 at 10 32 13 am
Screen Shot 2019-04-11 at 11 10 44 am

@2color

This comment has been minimized.

Copy link
Contributor

2color commented Apr 11, 2019

Another thing I noticed is the time being off for activity items from the token manager.
Screen Shot 2019-04-11 at 4 55 09 pm

bpierre added some commits Apr 11, 2019

Layout fixes + empty state
- Fix the icons.
- Add an empty state.
- Improve the layout of items.
- Move ActivityItem in its own component.

@sohkai sohkai referenced this pull request Apr 13, 2019

Closed

Feat/message signing #683

Activity Panel: autoclose mode
- A new component has been created, CombinedPanel, to manage the two panels.
- Refactor the onBlur management to close the Activity Panel.
- ActivityContext: attach the stored list to the context provider, and
  make sure it always exists.
@sohkai
Copy link
Member

sohkai left a comment

Will merge this with activity-panel so we only have one, updated branch to work off of. Let's make any other changes directly there or as PRs to there.

const interval = setInterval(() => {
const relativeTime = formatDistance(new Date(), new Date(date))
.replace('about', '')
.replace('minutes', 'min')
.trim()

This comment has been minimized.

Copy link
@sohkai

sohkai Apr 14, 2019

Member

Could we use formatDistanceStrict()?

There are some other edge cases like less than, etc.

{...props}
unreadActivityCount={unreadActivityCount}
onClearActivities={requestClearActivities}
onMarkActivitiesRead={requestMarkActivitiesRead}

This comment has been minimized.

Copy link
@sohkai

sohkai Apr 14, 2019

Member

Could we move all of these to be at the ActivityPanel level instead, since that's where they're needed?

We use onMarkActivitiesRead() here, but it seems like a good candidate to move to a componentDidUpdate() based on the open prop in ActivityPanel.

@sohkai sohkai merged commit 3a8c825 into activity-panel Apr 14, 2019

4 checks passed

License Compliance All checks passed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details

@sohkai sohkai deleted the activity-panel-ui branch Apr 14, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.