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

Switch from user placeholders to identicon #1193

Merged
merged 3 commits into from Oct 10, 2017

Conversation

Projects
None yet
3 participants
@noirbizarre
Copy link
Member

commented Oct 3, 2017

This pull request replace user placeholders by identicons (aka. a visual representation of a hash).

Algorithm

The user identifier is used as value to be hashed (not the email as it is not public and so not usable from external applications and also because it can change over the time).

Customization

The avatar/identicon backend is customizable:

  • you can switch implementations (using the AVATAR_PROVIDER)
  • you can configure each implementation when possible
  • you can create your own (using the udata.avatars python entrypoint)

Each parameter comes with a default value, overridable by theme and then local config

Implementations

This PR comes with 3 different implementations.

internal (default)

This implementation use pyidenticon to generate user avatars. This is udata which is generating the avatar and so there is no external request when displaying them.

Parameters are the follwing:

  • AVATAR_INTERNAL_SIZE (default: 7): Number of blocks (the matrix size) used by the internal provider. (Ex: 7 will render avatars on a 7x7 matrix)
  • AVATAR_INTERNAL_FOREGROUND (default: ['rgb(45,79,255)', 'rgb(254,180,44)', 'rgb(226,121,234)', 'rgb(30,179,253)', 'rgb(232,77,65)', 'rgb(49,203,115)', 'rgb(141,69,170)']): A list of foreground colors used by the internal provider to render the avatars
  • AVATAR_INTERNAL_BACKGROUND (default: 'rgb(224,224,224)'): The background color used by the internal provider
  • AVATAR_INTERNAL_PADDING (default: 10): The padding (in percent) used by the internal provider

screenshot-www data dev-2017-10-03-12-36-57-183
screenshot-www data dev-2017-10-03-14-03-16-141
screenshot-www data dev-2017-10-03-14-14-53-775

robohash

A provider redirecting to robohash avatars.

Parameters are:

  • AVATAR_ROBOHASH_SKIN *(default: 'set1'): The skin (set) used by the robohash provider.
  • AVATAR_ROBOHASH_BACKGROUND *(default: 'bg0' (transparent background)): The background used by the robohash provider.

screenshot-www data dev-2017-10-03-12-40-39-639
screenshot-www data dev-2017-10-03-14-05-04-871
screenshot-www data dev-2017-10-03-14-05-41-966

adorable

A provider redirecting to Adorable avatars.
This provider is not customizable.

screenshot-www data dev-2017-10-03-12-37-45-975
screenshot-www data dev-2017-10-03-14-04-16-819
screenshot-www data dev-2017-10-03-14-06-58-564

Fixes

This PR fixes:

  • the avatar display in user cards (use same spacing as dataset and reuse cards)
  • the spacing and sizing on the followers display on reuse page (see above screenshots)

Open discussions

Naming

It's hard to find a good wording for variables and helpers. Sometimes identicon comes, sometime it's avatar. Any suggestion is welcome on this issue/subject...

Packaging

This feature is located in the udata.features.identicon but the API is /avatars/<identifier>/<size>.
Given the previous point, it might be enhencable. Suggestions are welcome.

Caching

This kind of feature requires cache, the question is wether it's the udata responsibility or the reverse proxy.

@noirbizarre noirbizarre added this to the 1.2.0 milestone Oct 3, 2017

@noirbizarre noirbizarre requested a review from opendatateam/etalab Oct 3, 2017

@noirbizarre noirbizarre force-pushed the noirbizarre:dgfr-27-identicon branch 3 times, most recently from b9d9938 to 9ff29ba Oct 3, 2017

@abulte
Copy link
Member

left a comment

👌

@@ -298,6 +298,63 @@ This activates metrics, this is deactivated for tests

uData use Flask-FS as storage abstraction.

## Avatars/identicon configuration

Theses settings allows you to customize avatar rendition.

This comment has been minimized.

Copy link
@abulte

abulte Oct 3, 2017

Member

s/allows/allow/

This comment has been minimized.

Copy link
@abulte

abulte Oct 3, 2017

Member

Rendering or generation instead of rendition?

## Avatars/identicon configuration

Theses settings allows you to customize avatar rendition.
If defined to anything else than a flasy value, theses settings takes precedence over the theme configuration and the default values.

This comment has been minimized.

Copy link
@abulte

abulte Oct 3, 2017

Member

s/flasy/falsy/

This comment has been minimized.

Copy link
@abulte

abulte Oct 3, 2017

Member

s/takes/take/


Avatar provider used to render user avatars.

udata provides 3 backends:

This comment has been minimized.

Copy link
@abulte

abulte Oct 3, 2017

Member

s/udata/uData/

This comment has been minimized.

Copy link
@abulte

abulte Oct 3, 2017

Member

And below ⬇️


Theme can provide settings for the avatar provider.

These settings takes precedance over default values but are still overridables by local settings.

This comment has been minimized.

Copy link
@abulte

abulte Oct 3, 2017

Member

s/overridables/overridable/

This comment has been minimized.

Copy link
@abulte

abulte Oct 3, 2017

Member

s/takes/take/



# Default values overriden by theme default and local config
DEFAULTS = {

This comment has been minimized.

Copy link
@abulte

abulte Oct 3, 2017

Member

Why aren't the defaults stored in settings.py?

This comment has been minimized.

Copy link
@noirbizarre

noirbizarre Oct 3, 2017

Author Member

To let the theme bring custom parameters.

width="{size}" height="{size}"/>
</a>
'''.format(
markup = ''.join((

This comment has been minimized.

Copy link
@abulte

abulte Oct 3, 2017

Member

I liked the multiline string better but YMMV.

This comment has been minimized.

Copy link
@noirbizarre

noirbizarre Oct 3, 2017

Author Member

Yep me too but the extras spaces where the source of the visual breakage 😢

<img src="{{ user|avatar_url(48) }}" alt="{{ user.fullname }}"
width="48" height="48">
<img src="{{ user|avatar_url(60) }}" alt="{{ user.fullname }}"
width="60" height="60">

This comment has been minimized.

Copy link
@abulte

abulte Oct 3, 2017

Member

Shouldn't we deal with Retina (here and everywhere else)? i.e. request a 120px avatar.

This comment has been minimized.

Copy link
@noirbizarre

noirbizarre Oct 3, 2017

Author Member

Yes, but I think it requires a PR on its own (maybe dealing with srcset)
HiDPI (Retina is an Apple only commercial term 😜 ) support affects avatars but also any image rendered (org logo, reuse image...).

@noirbizarre noirbizarre force-pushed the noirbizarre:dgfr-27-identicon branch from 9ff29ba to 9bf3590 Oct 3, 2017

@oncletom

This comment has been minimized.

Copy link
Contributor

commented Oct 3, 2017

Nice finding!

The internal works well in thread! In a grid, it feels very heavy and more like an 8bits space invader clone rather than data.gouv.fr :-( Using the main colors of data.gouv.fr palette would probably remove the 'out of place' / 'in my face' feeling they give.

The robohash feels out of place with the voice and tone of data.gouv.fr — could be good in other udata contexts though.

adorable is uneven and I feel uncomfortable seeing some faces on data.gouv.fr — like the angry and semi-drunk ones.


I have used http://webcolourdata.com to extract the colour palette of data.gouv.fr, @noirbizarre could you try with these internal foreground colours?

  • #5685C4
  • #483D68
  • #483D68
  • #565D67
  • #016D1B
  • #125E9D
  • #E8512A
@noirbizarre

This comment has been minimized.

Copy link
Member Author

commented Oct 3, 2017

I tottaly agree but this PR is only a kind of framework:

  • robohash and adorable backends are simple examples of what is easily doable
  • theme can customize the colors but default values are the default from pyidenticon (your proposal should go in the gouvfr theme)
@noirbizarre

This comment has been minimized.

Copy link
Member Author

commented Oct 3, 2017

I joined the followers screenshot because it's something fixed in this PR.

But given:
screenshot-www data dev-2017-10-03-15-54-45-588

Most of the time it will look like:
screenshot-www data dev-2017-10-03-15-58-58-127

Maybe we can fix the display for reuses having a lots of followers (in another pr):

  • only display the first ones followed by a "and XX more" label with an expander
  • add a separation between avatars
  • ...??

@noirbizarre noirbizarre force-pushed the noirbizarre:dgfr-27-identicon branch 6 times, most recently from 41119a4 to 03b39e3 Oct 4, 2017

@abulte

abulte approved these changes Oct 9, 2017

@noirbizarre noirbizarre force-pushed the noirbizarre:dgfr-27-identicon branch from 03b39e3 to 89b6706 Oct 9, 2017

@oncletom
Copy link
Contributor

left a comment

I took some time to see the proposition again and I am not at ease with the generated adorable identicons. Nor I see any of them being "professional" in udata contexts I am aware of (like data.gouv.fr or opendata.lu).

I am not sure depending on a third party to display a low value feature (some avatars, compared to the rest of the project) is something we would like to enforce or recommend.

I understand the work has been done already and I don't intend to block the merge of the PR. Maybe picturing it in context would help. Don't know. Happy to talk about it.

@abulte

This comment has been minimized.

Copy link
Member

commented Oct 9, 2017

internal (pyidenticon) should be what's deployed, right? I'm not OK with adorable either ;-) But pyidenticon is OK for me. Maybe with a color PR on udata-gouvfr as suggested.

@noirbizarre

This comment has been minimized.

Copy link
Member Author

commented Oct 9, 2017

As stated before, the adorable avatars and the robohash avatars are simple examples (2 lines of code for adorable and 4 lines for robohash) of what's possible with this approach.
I can remove the lines but experience showed me that each time something is customizable (on udata or any other project), people ask for examples. This time I already packaged the examples. As I'm also doing the support, this allows me to give a simple github URL with samples.

depending on a third party: it's configuration, It's not the default one, and there is no dependency at all (the 2 implementations are simple HTTP redirects, no third party libraries or anything like this).

@noirbizarre noirbizarre force-pushed the noirbizarre:dgfr-27-identicon branch from 89b6706 to 70e8872 Oct 9, 2017

@oncletom

This comment has been minimized.

Copy link
Contributor

commented Oct 10, 2017

OK that's the bit I did not get initially. Thank for taking the time to rephrase it — I thought we only had a choice among 3 configurations. Now I understand identicon is a configurable system onto which we can plug any other avatar system, including our own.

@noirbizarre noirbizarre merged commit c5bcfe4 into opendatateam:master Oct 10, 2017

3 checks passed

ci/circleci: assets Your tests passed on CircleCI!
Details
ci/circleci: dist Your tests passed on CircleCI!
Details
ci/circleci: python Your tests passed on CircleCI!
Details

@noirbizarre noirbizarre deleted the noirbizarre:dgfr-27-identicon branch Oct 10, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.