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

Styled header/subtitles and "sensors" #27

Merged
merged 3 commits into from
Jul 6, 2017
Merged

Conversation

alexelash
Copy link
Contributor

Overview

This PR addresses the need to style metrics, headers/subtitles, and the "sensors" page suggested in issue #24.

To do make these changes, I also added an assets/ folder that contains League Gothic Condensed (an open source typeface that resembles the one used elsewhere in the application) and two illustration options for the Sensors page.

Screenshots

Turbidity with reading from the Schuylkill

turbidity_schuylkill

Turbidity with reading from the Delaware

turbidity_bu_delaware

Turbidity with no sensor reading

turbidity_bu_none

Sensor (option 1)

sensor_1

Sensor (option 2)

sensor_2

Notes

  • Working with Scott and Arianna, we shortened the text down for each screen.
  • To style the Sensors page, I tried to use the if(!metric) statement within window.onload in order to give the page's .container a class name of "sensor". Please let me know if this was a bad way to do this!
  • I'm not sure if I need all of the fallbacks I included in my @font-face declaration.
  • If neither illustration fit what you imagined for Sensors, please let me know, @ajrobbins!

Testing

  • Get this branch, then server
  • Load the spokes without making any changes and verify that you see data for the Schuylkill and appropriate headers and subscripts
  • Drop .usgs from the PRIMARY_SENSOR_URL constant, then refresh the app. visit the spokes and verify that you see the values, headers, and subscripts from the Delaware River.
  • Drop .usgs from the BACKUP_SENSOR_URL const, refresh the app, then visit the spokes again. Verify that you see the fallback values and text.
  • Switch out the illustration being used by changing background-image: url("assets/Sensor_Illustration_opt2.png"); to background-image: url("assets/Sensor_Illustration_opt1.png");
    `

@alexelash alexelash requested review from kellyi and ajrobbins July 5, 2017 17:46
@alexelash alexelash changed the title Styled Styled header/subtitles and "sensors" Jul 5, 2017
@ajrobbins
Copy link
Contributor

I love the styling on the metrics (top three screenshots). I'm sending the images to the client for feedback on text.

For the sensors illustration, I like option 1 but think the line brightness and weight (as perceived on my laptop) clash a bit with the rest of the graphic outside. Can you reduce the opacity or otherwise de-emphasize the lines?

@alexelash
Copy link
Contributor Author

@ajrobbins Sounds good! I'll keep working on option 1 per your suggestions and post a screenshot of it here for more feedback.

@alexelash
Copy link
Contributor Author

alexelash commented Jul 5, 2017

@ajrobbins Is this any better?
screen shot 2017-07-05 at 3 36 06 pm
or
screen shot 2017-07-05 at 3 38 29 pm

@alexelash
Copy link
Contributor Author

Newest Illustration in context:
screen shot 2017-07-05 at 5 00 09 pm

@kellyi
Copy link
Contributor

kellyi commented Jul 5, 2017

Taking a look at this!

@kellyi
Copy link
Contributor

kellyi commented Jul 5, 2017

It looks like the #sensor-data:after line's being drawn before the data returns, so it seems to show up at the top of the div before moving to the center when the number and subscript come back; read up on :after and since the div exists on loading the page, it looks like it's drawing the line immediately after the empty div.

Is there some good way to have it hidden initially and then to unhide it when writing data to the document here?: https://github.com/azavea/pwd-river-dispatches/blob/develop/usgs-details.html#L211

// Sensor lacks metrics, so giving its div.container a name of "sensor"
// in order to apply some specific styles.
var container = document.getElementsByClassName('container');
container[0].setAttribute('name', 'sensor');
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like it works well! I was able to load and unload the different spokes and this shows up only when the sensor spoke's selected.

I wonder if it'd make sense to give the container div above a unique string like sensor-data-container for its id attribute so that we could use something like

var container = documentGetElementById('sensor-data-container');
container.setAttribute('name', 'sensor');

to get the single element and then not have to use the array index here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense! I'm not super sure how to go about doing that (my javascript skills are that I can mostly read it, haha) so the revisions to the commit I put up won't include that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good! I tested this out and it works as is.

@@ -239,14 +292,21 @@
xhr.send();
}



Copy link
Contributor

Choose a reason for hiding this comment

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

There are few extra lines of whitespace here, plus one on 309. We usually try to avoid committing extra whitespace, so it may be worth dropping the extra lines in a fixup commit.

@kellyi
Copy link
Contributor

kellyi commented Jul 5, 2017

Here's a screenshot of the horizontal line thing; it shows up while the request is being made/before there's new data to display:

screen shot 2017-07-05 at 5 49 22 pm 3 2

@kellyi
Copy link
Contributor

kellyi commented Jul 5, 2017

Looks like the :after line might be showing up at the top of the sensor illustration, too:

screen shot 2017-07-05 at 5 53 04 pm

Wonder if we should just hide that div altogether for the illustration spoke.

@kellyi
Copy link
Contributor

kellyi commented Jul 5, 2017

Primary, backup, and fallback value's displaying correctly with the new styles!

@alexelash
Copy link
Contributor Author

@kellyi I hopefully got rid of all of the areas with unnecessary whitespace. To hide the white line unless sensor data has loaded, I'm checking whether or the div has any content yet using :not(:empty).

@ajrobbins
Copy link
Contributor

Victoria likes the design and language on the metric displays! Called them "legible and simple". Now I'm just checking with her on the sensor illustration.

@ajrobbins
Copy link
Contributor

Accidentally hit the "close and comment" button instead of "comment", apologies.

@ajrobbins
Copy link
Contributor

Victoria says the illustration is "spot-on". Nice job, @alexelash

Copy link
Contributor

@kellyi kellyi left a comment

Choose a reason for hiding this comment

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

+1. Nice job with this!

@alexelash alexelash merged commit 718628c into develop Jul 6, 2017
@alexelash alexelash deleted the al/restyle-sensor-text branch July 6, 2017 14:03
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.

4 participants