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.

Sign up for GitHub

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

Jw grid font #1191

Merged
merged 8 commits into from
Aug 15, 2017
Merged

Jw grid font #1191

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
eb76aca
Changes to increase the text and X-Axis and Y-Axis font
Aug 3, 2017
e916054
Merge branch 'master' into jw-grid-font
Aug 3, 2017
f1ca974
Fix to Server JS Lint failures
Aug 3, 2017
96ddf21
Changes to increase the text and X-Axis and Y-Axis font
Aug 3, 2017
2fb5971
Fix to Server JS Lint failures
Aug 3, 2017
a8d1eb3
Merge branch 'jw-grid-font' of https://github.com/18F/crime-data-fron…
Aug 9, 2017
612461f
Grids Font size change
Aug 9, 2017
cf723e7
Merge branch 'master' into jw-grid-font
jpwentz Aug 11, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion env.sample
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
export API_KEY="get your key at https://api.data.gov/signup/"
export API_KEY="get your key at https://api.data.gov/signup"
export CDE_API="https://api.usa.gov/crime/fbi/ucr"
export PORT=6005
2 changes: 1 addition & 1 deletion src/components/TrendChart.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,7 @@ class TrendChart extends React.Component {
until={until}
/>
<div className="mb2 clearfix">
<div className="sm-col mb1 sm-m0 fs-10 bold monospace black">
<div className="sm-col mb1 sm-m0 fs-14 bold monospace black">
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 14px might be a bit big given the rest of the surrounding content's font sizes:

10px
screen shot 2017-08-07 at 11 37 40 pm

12px
screen shot 2017-08-07 at 11 37 57 pm

14px
screen shot 2017-08-07 at 11 38 32 pm

I'd vote upping the font size to 12px for now.

BTW, @brendansudol great work on that alternating X axis label hiding! It looks just as good at 10px as 14px:

Rate per 100,000 people, by year
</div>
{crime === 'rape' && <TrendChartRapeLegend />}
Expand Down
2 changes: 1 addition & 1 deletion src/components/XAxis.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ const XAxis = ({ active, tickCt, tickSizeOuter, height, scale, showLine }) => {
className={`tick ${String(v) === String(active) ? 'bold' : ''}`}
>
<line stroke="#000" y2="6" />
<text fill="#000" x="0" y="12" dy=".71em">{format(v)}</text>
<text className="fs-14" fill="#000" x="0" y="12" dy=".71em">{format(v)}</text>
</g>
)
})
Expand Down
2 changes: 1 addition & 1 deletion src/components/YAxis.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ const YAxis = ({ tickCt, scale, width }) => {
return (
<g key={i} transform={`translate(0, ${pos})`} className="tick">
<line x2={width} y2="0" strokeOpacity={i === 0 ? '1' : '.25'} />
<text fill="#000" x="-32" y="0" dy=".32em">{fmt(v)}</text>
<text className="fs-14" fill="#000" x="-32" y="0" dy=".32em">{fmt(v)}</text>
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a totally fine approach, but we currently set the font size of the .axis elements in _viz.scss. I think we should either refactor out the parts of .axis that we already have classes for or just adjust the font-size in .axis.

To my mind, the tradeoff is that this style of css leads to a lot more stuff in the HTML attributes and so for SVG it might be nice to keep the styling in component style classes (like .axis) and for the rest of the HTML we can use the simple classes approach.

On the other hand, it makes a ton of sense to keep things as consistent as possible across the code base which is a compelling argument for refactoring out the aspects of .axis that we have elsewhere.

@jpwentz which approach do you prefer?

</g>
)
})
Expand Down