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

Production Consumption Toggle / mobile modal fix #1659

Merged
merged 14 commits into from Nov 2, 2018

Conversation

Projects
None yet
3 participants
@ovbm
Copy link
Contributor

commented Oct 30, 2018

the toggle on mobile:

notice that I removed the top electricitymap logo to gain some vertical space and instead placed the logo in the tab of the app. I think this solution makes a lot of sense for mobile:
emapanim

What's missing is a modal or legend that explains what's happening, maybe with a link to a blogpost for further info. Can someone make a suggestion for this text?

todo:

  • explanation moda/legend

@ovbm ovbm requested a review from corradio Oct 30, 2018

@ovbm ovbm self-assigned this Oct 30, 2018

@ovbm ovbm requested a review from brunolajoie Oct 30, 2018

@corradio

This comment has been minimized.

Copy link
Member

commented Oct 30, 2018

Smart move. Small detail: I see an vertical alignment issue with the caption of the button compared to the two other buttons.
Do you think we should rename "Areas"?
For the modal, we don't have any blogpost, but for consumption we could say "takes into account imports and exports" and for production "ignores imports and exports"

corradio added some commits Oct 30, 2018

Show resolved Hide resolved web/src/main.js
Show resolved Hide resolved web/src/main.js Outdated
@ovbm

This comment has been minimized.

Copy link
Contributor Author

commented Oct 30, 2018

The vertical alignment was always there, it moves the "active" legend item a couple of pixels up compared to the inactive ones. not a fan but I kept it because it was already there.

Don't know about Areas, do you have an Idea?

Ok, so it's going to be a hover label with those two infos. I wonder if the toggle is too prominent and if it should be collapsed.

@ovbm

This comment has been minimized.

Copy link
Contributor Author

commented Oct 30, 2018

I think default should be consumption view. It seems like now it's production

@ovbm

This comment has been minimized.

Copy link
Contributor Author

commented Oct 30, 2018

this is a solution for the mobile itunes thing:

with bar:
screenshot 2018-10-30 at 10 52 27

without bar:
screenshot 2018-10-30 at 10 52 49

@corradio

This comment has been minimized.

Copy link
Member

commented Oct 30, 2018

yes indeed default should be consumption. OK for the other changes.

@corradio

This comment has been minimized.

Copy link
Member

commented Oct 30, 2018

you should take a look at the reducer where default values are set

ovbm added some commits Oct 30, 2018

@ovbm

This comment has been minimized.

Copy link
Contributor Author

commented Oct 30, 2018

I think my work is done for this now

@corradio
Copy link
Member

left a comment

I pushed a small fix to account for initial values.
When observing a state change, it's always better to update the component based on the redux state, compared to toggling based on the component state. That way we are always sure the component is up to date no matter the initial value of the component.

@brunolajoie

This comment has been minimized.

Copy link
Collaborator

commented Oct 31, 2018

There's one more thing that will need update:

image

And same on the carbon emission view: remove imports
image

@corradio

This comment has been minimized.

Copy link
Member

commented Nov 2, 2018

Merging now, we can always improve later.

@corradio corradio merged commit 04fecfc into master Nov 2, 2018

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@corradio corradio deleted the ob/prod_cons_toggle branch Nov 2, 2018

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.