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

25 homepage news aggregator #26

Merged
merged 5 commits into from
Aug 5, 2019
Merged

25 homepage news aggregator #26

merged 5 commits into from
Aug 5, 2019

Conversation

benlk
Copy link
Contributor

@benlk benlk commented Aug 3, 2019

Changes

Note that merging this PR effectively merges #25, because styles in here build off of styles in those commits.

This pull request makes the following changes:

  • emboldens, centers and embiggens the widget titles in the homepage bottom area
  • applies styles to the Link Roundups widget.

Screen Shot 2019-08-02 at 20 55 35

Collapses down to, at its smallest,
Screen Shot 2019-08-02 at 20 55 22

Below 700px, it's two-column:

Screen Shot 2019-08-02 at 20 55 28

Why

For #16

Testing/Questions

Features that this PR affects:

  • homepage

Questions that need to be answered before merging:

  • is there any better way this could have been styled, than just kinda randomly floating things? Could CSS Grid have solved this problem?
  • the widget only adds thumbnail-sized images. How hard do we want to try to get images of a different aspect ratio?
  • make headline text bigger/smaller?

Steps to test this PR:

  1. Install link-roundups plugin
  2. create n>4 saved links with thumbnails
  3. add to the Homepage Bottom widget area a Saved Links widget set to 4, with thumbnail, no readmore, no excerpt

@benlk
Copy link
Contributor Author

benlk commented Aug 3, 2019

1h11m used of 2h

@joshdarby
Copy link

joshdarby commented Aug 5, 2019

@benlk

make headline text bigger/smaller?

I think it's ok since it matches what's in the mockup.

the widget only adds thumbnail-sized images. How hard do we want to try to get images of a different aspect ratio?

I would think it'd be preferable to show bigger images, but I'll leave that up to @MirandaEcho.

Other than that, it looks good and works well.

@MirandaEcho
Copy link

@joshdarby - between development and review, how much time has been logged for this task, and what would it take to resize the images?

@kaylima - from a design perspective, what are your thoughts on thumbnail vs. wider images?

@joshdarby
Copy link

@MirandaEcho I've put in 30 minutes of review time and it looks like @benlk has spent 1 hr 11 minutes. So total of 1 hr 40ish minutes have been used so far.

@benlk
Copy link
Contributor Author

benlk commented Aug 5, 2019

@MirandaEcho resizing the images would require, at minimum:

  • Copying the Link Roundups widget code over from that plugin, and modifying it so it doesn't conflict with that plugin's widget code
  • Modifying the widget to output a wider image
  • Check styles

I think that's about an hour's work.

@MirandaEcho
Copy link

Ok, given remaining time lets not do that for now, unless requested by the client or @kaylima from a design perspective.

@kaylima
Copy link
Member

kaylima commented Aug 5, 2019

Let's leave the thumbnails as-is for now, but please do the following:

decrease the space between the title and the organization (i.e. bring "ProPublica" up closer to the title)

Q: can we cap the title length to keep it more streamlined? 2 lines perhaps plus a "..."?

@benlk
Copy link
Contributor Author

benlk commented Aug 5, 2019

Truncation or abbreviation of the headlines is something better done by Workday Minnesota with editorial judgement involved, rather than a plain "cut it off after n characters" rule.

If we wanted to do truncation based on the number of lines, because the line length is variable at different screen sizes, we'd still see wrapping without a frontend JS solution to enforce truncation at a specific number of lines.

But beyond that, truncation is bad like homepage carousels are bad.

@kaylima
Copy link
Member

kaylima commented Aug 5, 2019

@benlk this is different because it's an aggregator and not their articles.

But, for sake of time, please just decrease the space between the title and the organization (i.e. bring "ProPublica" up closer to the title) and get on staging. thanks.

@benlk benlk merged commit bbe4aba into master Aug 5, 2019
@benlk benlk deleted the 25-homepage-news-aggregator branch August 5, 2019 21:17
@benlk benlk mentioned this pull request Aug 5, 2019
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