Opened 6 years ago

Closed 6 years ago

#332 closed change (fixed)

Add country flags to Eyeo team page

Reported by: greiner Assignee: greiner
Priority: P3 Milestone:
Module: Infrastructure Keywords: eyeo
Cc: trev, thetonymaster Blocked By:
Blocking: Platform:
Ready: yes Confidential: no
Tester: Verified working: no
Review URL(s):

http://codereview.adblockplus.org/5097747367591936/

Description

Background

We are a highly international team and proud of the diversity of nationalities across all team members.

What to change

Add a country flag next to each team member on the Eyeo team page to show how international we are.

Attachments (3)

eyeo-team-flags_public.png (270.2 KB) - added by greiner 6 years ago.
Design
patch_web.eyeo.com.diff (9.3 MB) - added by ileonelperea 6 years ago.
scripts_patch.diff (347 bytes) - added by thetonymaster 6 years ago.

Download all attachments as: .zip

Change History (12)

comment:1 Changed 6 years ago by trev

  • Cc trev added

Changed 6 years ago by greiner

Design

comment:2 Changed 6 years ago by greiner

  • Review URL(s) modified (diff)
  • Status changed from new to reviewing

Changed 6 years ago by ileonelperea

comment:3 follow-up: Changed 6 years ago by thetonymaster

We've added the flags and a css library to insert the flags. The flags are svg files and are lots of them. We modified the code so you put the nationality of each of the member staff abbreviated to two letters, in this patch, for example it defaults to "ru". The patch is Attachment patch_web.eyeo.com.diff, we got an error so we uploaded it again, but it seems it uploaded it okay in the first place.

On sitescripts we modified the mime types, it gives a application/onctet-stream, now it gives the correct type.

Changed 6 years ago by thetonymaster

comment:4 in reply to: ↑ 3 Changed 6 years ago by trev

Replying to thetonymaster:

We've added the flags and a css library to insert the flags.

Thank you for these patches. However, this issue already has an assignee - greiner is working on it and created his own patch already. In future, feel free to ask me to assign an issue to you, this way duplicate effort will be avoided. We can also give you contributor status, then you will be able to assign issues yourself.

While we will go with greiner's changes I'll add some notes on your changes, just for the sake of the exercise. These are the issues I would note in a regular review:

  • Having both flag-icon-squared and flag-icon-rounded classes on the images makes no sense.
  • An image width that is dependent on the font-size (em rather than px) probably isn't a good decision here. While the flag images can scale arbitrarily, the profile images have a fixed size. The size of flag images relative to profile images should be fixed rather than dependent on font size.
  • In general, the CSS code is very complex compared to greiner's solution.
  • There should be no code that is commented out - the code should either be used or it shouldn't be there at all.
  • SVG isn't really the optimal solution here. I'm not so much worried about browser support, but the files are rather large and PNG will produce the same or better results with a smaller download. More importantly, PNG images can be easily combined into a single sprite which is something we should do eventually here.
  • It isn't clear where these images come from and what their license is. Some images have a comment stating that they have been released to public domain - most don't.
  • Adding a second global stylesheet for the flags adds one more request for every page for no good reason. That stylesheet is only used on the team page, it should only be included there.

comment:5 Changed 6 years ago by trev

One more issue: turned out that you didn't write that CSS file but rather took it from https://github.com/lipis/flag-icon-css. Please never (NEVER!) add code that you didn't write without clearly indicating where it came from and what license it is available under.

comment:6 Changed 6 years ago by trev

  • Cc thetonymaster added

comment:7 follow-up: Changed 6 years ago by thetonymaster

Hi, sorry about the last one, this are my first contributions to open source projects, anyway. In case I use stuff that other people wrote, how or where should I report where I put that code from? Should we avoid libraries that does not specify licenses at all?

Thank you.

comment:8 in reply to: ↑ 7 Changed 6 years ago by trev

Replying to thetonymaster:

In case I use stuff that other people wrote, how or where should I report where I put that code from? Should we avoid libraries that does not specify licenses at all?

Yes, if there is no license then you have to ask the author of the code explicitly to grant you a permission - something that is usually too complicated. If there is the license then it needs to be ensured that this license is compatible with GPLv3 license we are using. Normally, external code has a license header in the file. If not - you need to add one yourself. All in all, it's not exactly a simple topic.

comment:9 Changed 6 years ago by greiner

  • Resolution set to fixed
  • Status changed from reviewing to closed
Note: See TracTickets for help on using tickets.