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

1707 article page loading icon #1734

Merged
merged 9 commits into from
Sep 15, 2017
Merged

Conversation

relativityboy
Copy link
Contributor

@relativityboy relativityboy commented Sep 12, 2017

#1707

Larger version of standard circle spinner displays for FullPost view when full post content is not available.

Also added a console command function that can be called to turn on & off logging of api calls.

screen shot 2017-09-12 at 2 53 11 pm

Updated appearance after consulting with UI folks. No longer larger - now "blue" with white-space.
screen shot 2017-09-14 at 3 11 08 pm

@sneak
Copy link
Contributor

sneak commented Sep 14, 2017

ideally this would be two separate PRs; we generally try to avoid multi-issue PRs (spinner plus konami code updates).

that said, this looks fine otherwise. utACK - test thoroughly before deploying.

Copy link
Contributor

@sneak sneak left a comment

Choose a reason for hiding this comment

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

utACK

@valzav
Copy link
Contributor

valzav commented Sep 14, 2017

I would keep the small spinner to make it consistent with other places where we have spinners. Also the small one from a design perspective is better - kind of less intrusive, especially on mobile.

@relativityboy
Copy link
Contributor Author

relativityboy commented Sep 14, 2017

@valzav - You do have some good points. I think the large one is consistent with our use case. It's the same spinner, scaled up appropriately.

Peripheral items are def worthy of smaller spinners. Because the article content is what people are looking for when they land on the page, it's good for them to see a larger indicator of progress. The small spinner can get 'lost'. Also, when the content does load, the vertical component of the page layout changes less, which is less jarring to the user. That means a more pleasant experience, and hopefully greater retention. (Users just wanna have fun!)

I imagine we'll eventually move from a very generic spinner to one that indicates the kind of content that's loading.

@valzav
Copy link
Contributor

valzav commented Sep 14, 2017

Yes I see the rationale behind making it bigger, maybe for the sake of consistency make all spinners slightly bigger? Also instead of passing styles into the component I would just pass a parameter saying 'please show a bigger version of the spinner' - this way it would be easier to reuse a bigger version.

@relativityboy
Copy link
Contributor Author

And be more standard. Yeah.

@roadscape
Copy link
Contributor

By my count we use LoadingIndicator in at least 15 places, are you suggesting to make all of them bigger?

@relativityboy
Copy link
Contributor Author

@roadscape - I think a good interim would be to have a small/default (current) size, a medium, and a large size.

@roadscape
Copy link
Contributor

I'd recommend to not change any existing spinners (it's a lot of testing and out of scope anyway) -- and yeah just add one or two predefined size-styles.

@pkattera
Copy link
Contributor

The large spinner looks awkward and kinda exacerbates the misalignment of the heading to the tags and metadata below the empty page content. Let's go with the smaller sized spinner but to @relativityboy point, I'd be happy to increase the padding above and below the spinner so that the height taken up by the spinner area is about the same as in the screenshot above. To make it further more visually obvious, we could make the spinner blue?

@relativityboy
Copy link
Contributor Author

Ok. Changes have been retested in dev. Looking good.

@roadscape
Copy link
Contributor

IMO it looked better centered with more vertical padding. Although I'm finding it pretty hard to trigger this case and when I do the post loads pretty quick (even when I have Chrome throttle to 100kb/s and 1s latency).

@relativityboy
Copy link
Contributor Author

@roadscape - I get where you're coming from on the centering, but pon called for it to be left aligned, so we're going with that.

@roadscape roadscape merged commit b4a001b into master Sep 15, 2017
@roadscape roadscape deleted the 1707-article-page-loading-icon branch September 15, 2017 02:07
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

5 participants