Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Display timestamp in Signer requests details #2324

Merged
merged 2 commits into from Sep 27, 2016
Merged

Conversation

ngotchac
Copy link
Contributor

Display the request creation date in the Signer Requests details (#2283)

@ngotchac ngotchac added A0-pleasereview 🤓 Pull request needs code review. M5-ui labels Sep 26, 2016
@parity-cla-bot
Copy link

It looks like this contributor signed our Contributor License Agreement. 👍

Many thanks,

Ethcore CLA Bot


import ReactTooltip from 'react-tooltip';
import DescriptionIcon from 'material-ui/svg-icons/action/description';
import GasIcon from 'material-ui/svg-icons/maps/local-gas-station';
Copy link
Contributor

Choose a reason for hiding this comment

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

😝

<div className={ styles.expandedContainer }>
{ this.renderDataExpanded() }
</div>
<TransactionSecondaryDetails
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it really worth creating an etirely new component? I don't have strong opinions on this, but it feels bloated.

Copy link
Contributor

Choose a reason for hiding this comment

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

As it stands, for the Signer is is probably the best approach. Everything is very, very, very modular and re-used among the different Pending/Completed layouts. Best way of fitting in with the style that is there.


.dataTooltip {
word-wrap: break-word;
max-width: 400px;
Copy link
Contributor

@derhuerst derhuerst Sep 26, 2016

Choose a reason for hiding this comment

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

Using font-size-based units like em, ex, … for layout-related things will help increate modularity & accessibility.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed. However, this Signer is a nightmare as the layouts are, we need a separate issue for re-working the layouts. As-is this is the best approach to take here unless you want to break everything in the signer display.

Copy link
Contributor

Choose a reason for hiding this comment

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

fair enough

@@ -108,3 +108,8 @@ function setIsSending (pending, id, isSending) {
return p;
}).slice();
}

function addTimestamp (request) {
request.date = new Date();
Copy link
Contributor

Choose a reason for hiding this comment

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

This modifies the request object and can therefore later have unintended side effects. I'd propose (r) => {request: r, timestamp: new Date()}. If you don't need the request, (r) => new Date().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The thing is that this new format would imply many changes for Components using theses requests. Maybe add this timestamp field at a lower level, like request.payload.timestamp ?

@jacogr
Copy link
Contributor

jacogr commented Sep 26, 2016

@ngotchac Can I add and issue to fix here while you are digging around - it would seem like the Signer stuff doesn't pop up in most-recent first on-top. It should be a simple fix, not sure why it goes awry on my side, I was sure it actually had the most recent on-top.

https://github.com/ethcore/parity/issues/2343

@jacogr jacogr added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Sep 27, 2016
@jacogr jacogr merged commit 2f72988 into js Sep 27, 2016
@ngotchac ngotchac deleted the js-signer-timestamps branch September 27, 2016 11:31
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants