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

Fix mobile CSS #1531

Merged
merged 9 commits into from Jul 30, 2018
Merged

Fix mobile CSS #1531

merged 9 commits into from Jul 30, 2018

Conversation

corradio
Copy link
Member

@corradio corradio commented Jul 28, 2018

Tested on:

  • Desktop Chrome
  • Cordova Android 5.1
  • iOS Chrome

On mobile devices, when WebGL isn't available, the "map" tab shouldn't be available.
There was a CSS bug that prevented that.

Also fixed some layout errors when clicking on an area without a parser.

Also addressed (hopefully) a layout bug on Android 5.1.

image

@corradio corradio requested a review from ovbm July 28, 2018 07:38
@@ -1173,8 +1169,7 @@ div.highscore-button:hover {
padding-top: 8px;
font-family: "Open Sans", sans-serif;
}
#tab.nomap #tab-content .list-item.map-button,
{
#tab.nomap #tab-content .list-item.map-button {
Copy link
Member Author

Choose a reason for hiding this comment

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

note: there was an extra comma that needed removal -_-

data-url="https://www.electricitymap.org"
data-via="electricitymap"
data-lang="<%= locale %>">
<p>
Copy link
Member Author

Choose a reason for hiding this comment

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

just reindent

</div>
<div class="mobile-faq"></div>
Copy link
Member Author

Choose a reason for hiding this comment

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

moved closing brace to upper line

@@ -2,7 +2,7 @@ const redux = require('redux');
const reduxLogger = require('redux-logger').logger;
const reducer = require('./reducers');

const isProduction = window.location.href.indexOf('electricitymap') !== -1;
const isProduction = window.location.href.indexOf('electricitymap') !== -1;
Copy link
Member Author

Choose a reason for hiding this comment

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

removed space

@@ -430,10 +429,7 @@ body {
}

.mobile-info-tab, .left-panel-zone-details {
display: flex;
Copy link
Member Author

Choose a reason for hiding this comment

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

required for mobile "about" tab to show correctly. Not sure why there was a column flex there.

@@ -1746,7 +1741,6 @@ div.highscore-button:hover {
}
.left-panel {
position: relative;
height: auto;
Copy link
Member Author

Choose a reason for hiding this comment

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

core of the bugfix 2/2

@corradio corradio changed the title Fix mobile CSS [WIP] Fix mobile CSS Jul 28, 2018
@corradio corradio removed the request for review from ovbm July 28, 2018 11:49
@@ -211,7 +211,7 @@ co2Sub = function(str) {
</div>
</div>
<div class="info-text">
<%- __('panel-initial-text.thisproject') %> <a href="https://github.com/tmrowco/electricitymap-contrib" target="_blank"><%- __('panel-initial-text.opensource') %></a> (<%- __('panel-initial-text.see') %> <a href="https://github.com/tmrowco/electricitymap-contrib#data-sources" target="_blank"><%- __('panel-initial-text.datasources') %></a>). <%- __('panel-initial-text.contribute', 'https://github.com/tmrowco/electricitymap#adding-a-new-country') %>.
<%- __('panel-initial-text.thisproject') %> <a href="https://github.com/tmrowco/electricitymap-contrib" target="_blank"><%- __('panel-initial-text.opensource') %></a> (<%- __('panel-initial-text.see') %> <a href="https://github.com/tmrowco/electricitymap-contrib#data-sources" target="_blank"><%- __('panel-initial-text.datasources') %></a>). <%- __('panel-initial-text.contribute', 'https://github.com/tmrowco/electricitymap#adding-a-new-country') %>.
Copy link
Member Author

Choose a reason for hiding this comment

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

re-indented

</div>
<div class="left-panel-zone-details">

<div class="left-panel-zone-details-toolbar">
Copy link
Member Author

Choose a reason for hiding this comment

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

moved the toolbar (country name and flag) into the header (which is always visible).
Not sure why this wasn't done before.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I remember this being a bit of a compromise to not have too much vertical space being taken up by fixed content, reducing the amount of info visible on smaller screens.

@@ -15,8 +15,6 @@ var moment = require('moment');
var flags = require('../helpers/flags');
var translation = require('../helpers/translation');

const allChildrenSelector = '.country-table-header,.country-show-emissions-wrap,.country-table-container,.country-history';
Copy link
Member Author

Choose a reason for hiding this comment

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

moved around and changed the list of elements that are hidden when a country has no parser associated.

@@ -312,6 +306,10 @@ co2Sub = function(str) {
<div class="country-table-container"></div>
</div>

<div class="zone-details-no-parser-message">
Copy link
Member Author

Choose a reason for hiding this comment

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

Moved the "no parser message" under the header

padding-right: calc(2rem - 6px);
padding-left: 2rem;
margin: 0;
-webkit-overflow-scrolling: touch;
}
.country-panel-wrap {
position: relative;
padding-top: 160px;
padding-top: 200px;
Copy link
Member Author

Choose a reason for hiding this comment

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

now that the header is included, more padding must be applied to left panel content

@@ -111,15 +111,15 @@ line, path {
}

.country-panel{
overflow-y: auto;
overflow-y: scroll;
Copy link
Member Author

Choose a reason for hiding this comment

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

scroll the country panel instead of the container

Copy link
Contributor

Choose a reason for hiding this comment

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

Did you test this on desktop? I think there was a specific reason for scrolling the container versus the panel. I think it would be nice to redo the html structure for the panel if we have time, now it's a bit of a patchwork and the lack of consistency is making it difficult to bugfix.

Copy link
Member Author

Choose a reason for hiding this comment

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

yes I tested it. I actually think it's better structured now. But let's review together

.left-panel-zone-details .detail-bottom-section{
padding: 0rem 1rem 1.25rem;
.detail-bottom-section{
flex: 1 0 auto;
Copy link
Member Author

Choose a reason for hiding this comment

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

remember to flex grow

@@ -1151,8 +1149,7 @@ div.highscore-button:hover {
-moz-box-shadow: 0px 0px 12px 0px rgba(0,0,0,0.10);
box-shadow: 0px 0px 12px 0px rgba(0,0,0,0.10);
z-index: 2;
border-top-right-radius: 8px;
border-top-left-radius: 8px;
flex: 1 0 auto;
Copy link
Member Author

Choose a reason for hiding this comment

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

remember to flex grow

@corradio corradio changed the title [WIP] Fix mobile CSS Fix mobile CSS Jul 28, 2018
@corradio corradio requested a review from ovbm July 28, 2018 19:44
@corradio
Copy link
Member Author

Now additionally tested in Safari and Firefox desktop + responsive mode

@ovbm ovbm merged commit 2e0a6c8 into master Jul 30, 2018
@ovbm ovbm deleted the olc/quickfix-mobile branch July 30, 2018 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants