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

Status page updates #3774

Merged
merged 13 commits into from
Dec 10, 2016
Merged

Status page updates #3774

merged 13 commits into from
Dec 10, 2016

Conversation

jacogr
Copy link
Contributor

@jacogr jacogr commented Dec 9, 2016

parity 2016-12-09 17-35-52

@jacogr jacogr added A0-pleasereview 🤓 Pull request needs code review. M7-ui labels Dec 9, 2016
@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 85.605% when pulling e2ffbe6 on jg-status-updates into 60ea787 on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.04%) to 85.56% when pulling c706879 on jg-status-updates into a6fcd8a on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.009%) to 85.612% when pulling 0fd9c14 on jg-status-updates into a6fcd8a on master.

@@ -18,7 +18,7 @@
.layout {
padding: 0.25em 0.25em 1em 0.25em;

> * {
margin-bottom: 0.75em;
&>div {
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason for that ? The & is useless, and we might not want to limit to divs. padding instead of margin is what was breaking the event pannel in Contracts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without that approach, you get a border around the full Page + Actionbar, instead of just the content below. So We are saying - only add border around the direct decendents.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Scrap the above, wrong comment, been too long between writing and review. Will need to go back and see why.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Swapped to margin.

I recall now. The reasson why this was originally introduced (not this PR actually, this was just supposed to move stuff around) it to not make the fixed statusbar at the bottom overlap with the page. Any other solution and you start having funnies across the different pages, e.g. dapps as well.

May be good to have a re-look at this in your fixes branch.

Copy link
Contributor

Choose a reason for hiding this comment

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

So this is supposed to be a property of the .page class and not its descendants. That's why there is padding: 0.25em 0.25em 1em 0.25em for the Page Component. Anyway, looks good now :)

const classes = `${styles.layout} ${className}`;
let actionbar = null;

if (title || buttons) {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@ngotchac ngotchac added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Dec 10, 2016
@ngotchac
Copy link
Contributor

Fixed some case-sensitivity issues too. Can't MacOS be set as case-sensitive ?

@jacogr
Copy link
Contributor Author

jacogr commented Dec 10, 2016

Driving me crazy a bit. Been using the tests/builds to pick the case sensitivity up.

@ngotchac ngotchac added A8-looksgood 🦄 Pull request is reviewed well. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Dec 10, 2016
@jacogr jacogr merged commit 173a888 into master Dec 10, 2016
@jacogr jacogr deleted the jg-status-updates branch December 10, 2016 21:21
@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 85.613% when pulling 5efd29a on jg-status-updates into 08a47ea on master.

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

3 participants