Jump to conversation
Unresolved conversations (0)
Nice work!

Nice work!

All of your conversations have been resolved.

Resolved conversations (15)
@silverwind silverwind Feb 6, 2020
Maybe we can introduce a helper function, similar to the go template one: ```js function svg(name, size) { return `<svg class="svg ${name}" width="${size}" height="${size}" aria-hidden="true"><use xlink:href="${staticPrefix}/img/svg/icons.svg#${name}"/></svg>`; }
Outdated
web_src/js/index.js
@silverwind silverwind Feb 6, 2020
```suggestion themes[parse(path).name] = [path]; ``` forgot to revert this one
Outdated
webpack.config.js
@sapk sapk Feb 6, 2020
Should we put an alt attr on the image ? (I am only asking because I don't know the exact answer and could be out off scope for this PR)
Outdated
modules/templates/helper.go
silverwind jolheiser
silverwind and John Olheiser
@sapk sapk Feb 6, 2020
Maybe .hidden instead of .invisible ? Doesn't semantic has a class already for this purpose ?
web_src/less/_base.less
sapk silverwind
jolheiser
Antoine GIRARD, silverwind, and John Olheiser
@sapk sapk Feb 6, 2020
We could maybe not add this file in git repo since it is build with js frontend build which is not stored anymore if I recall.
Outdated
public/img/svg/icons.svg
silverwind
silverwind
@sapk sapk Feb 6, 2020
If the icons.svg sprite will only contain octicon. I would suggest using `svg-octicon` as the helper name. Futhermore auto-prefix with octicon- the hash to not have repated at each calls.
modules/templates/helper.go
silverwind jolheiser
silverwind and John Olheiser
@sapk sapk Feb 6, 2020
Could you check the hash content of attr href of use element to keep the test goal ?
integrations/repo_test.go
jolheiser
John Olheiser
@silverwind silverwind Feb 6, 2020
Guess this can be removed now.
Outdated
Makefile
@silverwind silverwind Feb 5, 2020
Should be in `dependencies`: ``` sh npm uninstall @primer/octicons && npm i @primer/octicons
Outdated
package.json
@silverwind silverwind Feb 5, 2020
Could the `mega` argument be made a `interface` enabling a even shorter `{{octicon "lock"}}` for non-mega ones?
Outdated
modules/templates/helper.go
jolheiser silverwind
John Olheiser and silverwind
@techknowlogick techknowlogick Feb 5, 2020
I know this request will change a lot of your work below, however is it possible to use icon as the first argument in this function? The reason is that it is the most important argument, and that mega is a modifier. This isn't a blocker for me though.
Outdated
modules/templates/helper.go
jolheiser
John Olheiser
@silverwind silverwind Feb 2, 2020
I wouldn't define dimensions like that in CSS because that specific icon can appear in multiple places requireing different sizes. Just default either `svg` or a `.svg-icon` class to 16px in CSS. More specific sizes can be set either in template attributes or more specific CSS rules.
web_src/less/_svg.less
silverwind jolheiser
silverwind and John Olheiser
@silverwind silverwind Feb 2, 2020
Maybe a template helper would be nice, like `{{svg "mega-octicon" 30 30}}`. Arguments would be name,width,height. Width and height can default to a commonly used value like 16px. Height could default to Width when absent so `{{svg "mega-octicon" 30}}` could work too.
Outdated
templates/repo/header.tmpl
jolheiser silverwind
John Olheiser and silverwind
@guillep2k guillep2k Feb 2, 2020
Do icons come sorted in a reproducible way (e.g. alphabetically)?
Outdated
scripts/generate-octicons.go
jolheiser
John Olheiser
@6543 6543 Feb 2, 2020
```suggestion <svg class="mega-octicon-svg"><use xlink:href="{{StaticUrlPrefix}}/img/svg/octicons.svg#repo-template{{if .IsPrivate}}-private{{end}}" /></svg> ```
Outdated
templates/repo/header.tmpl