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

can set an icon on resources, and display it on pipeline and resource views #3581

Merged
merged 7 commits into from Apr 1, 2019

Conversation

@mockersf
Copy link
Contributor

mockersf commented Mar 25, 2019

for #3169

Icons can be specified on the resources:

resources:
  - name: repo.git
    type: git
    icon: https://about.gitlab.com/images/press/logo/png/gitlab-icon-rgb.png
    source:
      ...

Those icons are then displayed on the pipeline:
icon-pipeline

or on the resource:
icon-resource

@vito

This comment has been minimized.

Copy link
Member

vito commented Mar 25, 2019

Thanks for starting on this!

I like the end result but I have a few concerns with the approach:

  1. I'm not sure when I as a user would feel motivated enough to maintain these icons. I would rather have them show up automatically, e.g. determined by the resource type.
  2. It seems like a potential security hazard to introduce arbitrary outbound URLs from the pipeline. More on this here.
  3. Because there's no clear place to host these images yourself, it would result in a lot of hotlinking which those server admins may not be happy with (and hopefully they wouldn't retaliate by swapping those icons for something else).

Maybe I should just update concourse/rfcs#1 to include the first idea? The basic idea is that a resource types could emit a base64-encoded image from /info and Concourse would then serve the images itself. The resource type could maybe even return different icons for different services it's pointed to, e.g. the git resource could embed icons for GitLab,. GitHub,. BitBucket, vanilla git. etc.

@vito vito self-assigned this Mar 25, 2019
@mockersf

This comment has been minimized.

Copy link
Contributor Author

mockersf commented Mar 25, 2019

I would love to see resource types bring their own icons and I think it would be great to have it included in resources v2 !

This PR was more on how they can be displayed, so I picked the shortest path on how to add an icon to a resource.

I wasn't aware of the security issue, as (in our case), all users are completely trusted. Something like Font Awesome could be used to select the icon instead of allowing any URL. The resource definition would become, for gitlab icon:

resources:
  - name: repo.git
    type: git
    icon: fab fa-gitlab
    source:
      ...

I think having resource types bring their own icons, and allowing users to override them using Font Awesome icons would be great

@mockersf mockersf force-pushed the mockersf:resource-icon branch from 465cbb2 to 5cca3d5 Mar 26, 2019
@mockersf

This comment has been minimized.

Copy link
Contributor Author

mockersf commented Mar 26, 2019

I managed to do it using Font Awesome icons. Version 5 of Font Awesome lets you use it completely as svg instead of a font, which makes it easier to integrate to the pipeline view.

It looks almost the same but the icons are black and white which arguably looks better:
icon-pipeline
icon-resource
The icons also have better proportions as they don't have huge empty margins like the one from Gitlab / Slack official branding

@vito

This comment has been minimized.

Copy link
Member

vito commented Mar 26, 2019

Ooh, I like that idea actually; relying on Font Awesome would also ensure all the icons have a consistent visual narrative. Allowing resource types to bring in arbitrary images could make the UI pretty noisy/messy in the worst-case, so that would solve it nicely.

I like the idea, and since it solves the security problem I'd be less picky about whether this is part of the resource interface yet, so I'm happy to go forward with this PR. 👍

Minor suggestion: can we have the icon: field just be the gitlab portion? Not sure what the fab part is but at least the fa- prefixing we can probably do ourselves. That way if we ever want to switch from Font Awesome to something else we'd just need to map these icon names to the new icon.

@mockersf

This comment has been minimized.

Copy link
Contributor Author

mockersf commented Mar 27, 2019

The fab part is mandatory and can be either fab for Font Awesome brand (brand icons) or fas for Font Awesome solid (solid icons). We can remove it if we limit available icons to brands (or solid).

But if we leave the choice between brand and solid open, I think it's best to keep the complete prefix "fab fa-"gitlab as users can just copy paste from Font Awesome website

@vito

This comment has been minimized.

Copy link
Member

vito commented Mar 27, 2019

@mockersf Ah, that makes sense. I was worried about leaking implementation details like CSS classes up to the user but it sounds like that's a requirement for specifying the icon completely, and they might specify something other than fab. In that case it's probably best to just use the Font Awesome-documented classes rather than reinvent our own mapping. 👍

@Lindsayauchin

This comment has been minimized.

Copy link
Contributor

Lindsayauchin commented Mar 28, 2019

@mockersf @vito
In order to keep the icon set consistent with the Material design icon set we just updated the app to
can we pull the icons from this library?

https://materialdesignicons.com
https://github.com/Templarian/MaterialDesign

@mockersf mockersf force-pushed the mockersf:resource-icon branch from d5b822d to 55461f2 Mar 28, 2019
@mockersf

This comment has been minimized.

Copy link
Contributor Author

mockersf commented Mar 29, 2019

@Lindsayauchin yes, we can! It's now using icons from the Material design icon set.

specifying an icon with it doesn't need prefixes:

resources:
  - name: repo.git
    type: git
    icon: gitlab
    source:
      ...

As this icon set is not really packaged to be used as svg, I had to package it myself.
I used a small shell script to create the bulk of the package in web/public/mdi-svg-v3.5.95.js:

npm install @mdi/svg
echo "{" > mdi-svg.js; for file in node_modules/\@mdi/svg/svg/*; do cat $file | sed 's/^.* id="mdi-\([^"]*\)" .*path d="\([^"]*\)".*$/"\1":"\2",/' >> mdi-svg.js; done; echo "}" >> mdi-svg.js

and some manual changes after that to add js wrapper. It's not minified but that wouldn't change much given the content of the file.
@vito I'm not sure where to put this to document how to update the library

@mockersf mockersf force-pushed the mockersf:resource-icon branch from 55461f2 to 90f4109 Mar 29, 2019
@vito

This comment has been minimized.

Copy link
Member

vito commented Mar 29, 2019

@Lindsayauchin Oh neat, I didn't know MD had a bunch of branding and community-made icons too. Perfect!

@mockersf How about tracking that in yarn? We could add a dependency for @mdi/svg in package.json and add a yarn command for building the JS file using the script you just pasted here: https://github.com/concourse/concourse/blob/master/package.json#L25

@Lindsayauchin

This comment has been minimized.

Copy link
Contributor

Lindsayauchin commented Mar 29, 2019

@mockersf Can we make a couple polish updates to the UI that will improve the balance of the title with the icons?

  • keep the icon size to 24 x 24 px. This way we are not taking up too much screen real estate in that secondary bar.
  • add in a 10 px padding between the icon and the resource title.

Screen Shot 2019-03-29 at 11 16 07 AM

Screen Shot 2019-03-29 at 11 16 53 AM

@mockersf mockersf force-pushed the mockersf:resource-icon branch from 31cfc84 to d491444 Mar 29, 2019
@mockersf

This comment has been minimized.

Copy link
Contributor Author

mockersf commented Mar 29, 2019

@Lindsayauchin done !
icon-aligned

@vito

This comment has been minimized.

Copy link
Member

vito commented Mar 29, 2019

@mockersf Looks like this needs a rebase now that #3623 is merged. Sorry about that!

mockersf added 6 commits Mar 24, 2019
… view

Signed-off-by: François Mockers <mockersf@gmail.com>
Signed-off-by: François Mockers <mockersf@gmail.com>
Signed-off-by: François Mockers <mockersf@gmail.com>
Signed-off-by: François Mockers <mockersf@gmail.com>
Signed-off-by: François Mockers <mockersf@gmail.com>
Signed-off-by: François Mockers <mockersf@gmail.com>
@mockersf mockersf force-pushed the mockersf:resource-icon branch from d491444 to ce59ecc Mar 29, 2019
@mockersf

This comment has been minimized.

Copy link
Contributor Author

mockersf commented Mar 29, 2019

@vito and done too !

Signed-off-by: François Mockers <mockersf@gmail.com>
@vito
vito approved these changes Apr 1, 2019
Copy link
Member

vito left a comment

Tried it out locally, looks good - thanks a bunch! 👍

@vito vito merged commit 3d984df into concourse:master Apr 1, 2019
5 checks passed
5 checks passed
DCO DCO
Details
WIP Ready for review
Details
concourse-ci/testflight Concourse CI build success
Details
concourse-ci/unit Concourse CI build success
Details
concourse-ci/watsjs Concourse CI build success
Details
@marco-m

This comment has been minimized.

Copy link
Contributor

marco-m commented Apr 1, 2019

thanks @mockersf !

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