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

Add new "Icons" page for icons customisation and reuse across the admin interface #6028

Merged
merged 4 commits into from
Feb 10, 2023

Conversation

allcaps
Copy link
Member

@allcaps allcaps commented May 14, 2020

Docs draft for #4821

@allcaps allcaps self-assigned this May 14, 2020
Copy link
Contributor

@tomdyson tomdyson left a comment

Choose a reason for hiding this comment

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

Great docs @allcaps!

docs/advanced_topics/icons.rst Outdated Show resolved Hide resolved
docs/advanced_topics/icons.rst Outdated Show resolved Hide resolved
docs/advanced_topics/icons.rst Outdated Show resolved Hide resolved
docs/advanced_topics/icons.rst Outdated Show resolved Hide resolved
docs/advanced_topics/icons.rst Outdated Show resolved Hide resolved
docs/advanced_topics/icons.rst Outdated Show resolved Hide resolved
@allcaps allcaps marked this pull request as draft May 14, 2020 10:03
@allcaps allcaps force-pushed the svg-icon-docs branch 3 times, most recently from ea3aeff to 350d896 Compare May 14, 2020 10:58

The SVG should:

- Have a size of 16x16 points
Copy link
Contributor

Choose a reason for hiding this comment

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

It is unclear to me what 16x16 points means. Is it the viewbox?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. I added an example.

Copy link
Contributor

Choose a reason for hiding this comment

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

Cheers @allcaps!
I feel this may prove tricky for non-SVG proficient developers who just want to use a FontAwesome icon (e.g. https://fontawesome.com/icons/clipboard-list has a viewbox of 0 0 384 512 (and I, for example don't have the tools to convert it to a 0 0 16 16 viewbox. Changing the value ends up with a blank icon)

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure the same size matters that much as the final size is defined in CSS.
Maybe the ratio is significant. But I'm not sure about that too. I have to experiment.

Copy link
Contributor

Choose a reason for hiding this comment

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

aside from this line which is confusing, the rest LGTM and ready to be merged on my opinion

Copy link
Contributor

Choose a reason for hiding this comment

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

I am experiencing the same issue. Trying to use fork-awesome icons which look like this:

<svg width="1536" height="1536" xmlns="http://www.w3.org/2000/svg">
  <path d="M1401 1547l-6 6c-75 ... " />
</svg>

Simply removing the width, height, and xmlns attributes and adding viewbox="0 0 16 16" does not scale it correctly. I'm not sure what kind of tools or attributes should be used to convert an existing SVG into a wagtail-flavored SVG.

Base automatically changed from master to main March 3, 2021 17:08
@adsee42
Copy link

adsee42 commented Jun 23, 2021

Hi, it's been a year, any progress on this?
Ability to adding more icons would be really helpful!

@zerolab
Copy link
Contributor

zerolab commented Jun 23, 2021

Hey @adsee42,

The switch to SVG is mostly done. See #6107 for the full list and progress.

What would help get everything over the finish is line is submitting PRs for the remaining items, and reviewing/testing related pull requests such as this one.

@gasman
Copy link
Collaborator

gasman commented Aug 4, 2021

One thing we've tended to miss when adding icons to Wagtail is ensuring that any necessary attribution is in place (which is a requirement for Font Awesome v5 - see #7377). It would be good if we could find somewhere in these docs to put a reminder of this.

This might be something that's better suited for a page in "Contributing to Wagtail" though - it looks like these docs are primarily aimed at people adding icons for their own purposes (including third-party packages).

{% icon name="rocket" classname="..." title="Launch" %}


Icon font support
Copy link
Contributor

Choose a reason for hiding this comment

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

Should add a note here that icon fonts cannot be used for icon attributes on Streamfield blocks (not sure if this also applies to other icon attributes defined in Python code)

@zerolab
Copy link
Contributor

zerolab commented Nov 25, 2022

@allcaps this could use a rebase and I think it is high time we merge this in as menus, streamfields and quite a lot of UI components use the SVG icons now

@squash-labs
Copy link

squash-labs bot commented Dec 8, 2022

Manage this branch in Squash

Test this branch here: https://allcapssvg-icon-docs-1a1e7.squash.io

@allcaps
Copy link
Member Author

allcaps commented Dec 8, 2022

@zerolab Rebased!

I've included code snippets to give easy access to the available icon names and provide a example.

Should we drop the section about icon fonts?

lb- added a commit to lb-/wagtail that referenced this pull request Dec 9, 2022
- Preserve the existing `class_name` behaviour in most other cases
- Update only docs reference to use `classname`
- Relates to wagtail#6107 & wagtail#6028
lb- added a commit that referenced this pull request Dec 9, 2022
- Preserve the existing `class_name` behaviour in most other cases
- Update only docs reference to use `classname`
- Relates to #6107 & #6028
@allcaps allcaps force-pushed the svg-icon-docs branch 2 times, most recently from f6c8f75 to efb8911 Compare December 26, 2022 16:37
Copy link
Contributor

@zerolab zerolab left a comment

Choose a reason for hiding this comment

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

LGTM! :shipit:

Copy link
Member

@lb- lb- left a comment

Choose a reason for hiding this comment

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

Just had another read - also looks great. Thanks @allcaps !

@lb-
Copy link
Member

lb- commented Feb 7, 2023

Does anyone know why we did not get this into 4.2? I'm happy to merge now but not sure if we were still waiting on something.

@zerolab / @allcaps

Maybe it's worth putting this in 4.2 stable if we do merge.

@zerolab
Copy link
Contributor

zerolab commented Feb 7, 2023

@lb- uh-oh, I have failed to add to the 4.2 milestone, so it got lost in translation.

I don't see any reason not to, but pinging @gasman

@gasman
Copy link
Collaborator

gasman commented Feb 7, 2023

@zerolab Go for it 👍

@thibaudcolas thibaudcolas added this to the 5.0 milestone Feb 10, 2023
@thibaudcolas
Copy link
Member

Merging now! I believe there is still work to be done on this to be honest, but we’re hoping to push for this to happen by the next release, and if this fails we can add a disclaimer.

I wouldn’t recommend us backporting this. The documented setup here isn’t compatible with all of our icons, which is part of the reason why #6107 is still only halfway through the list. We have two solid approaches to solving this mentioned in #7511 that should work without changes to how icons are registered, but they’re just ideas/prototypes at this stage.

@thibaudcolas thibaudcolas changed the title Add SVG icon docs Add new "Icons" page for icons customisation and reuse across the admin interface Feb 10, 2023
@thibaudcolas thibaudcolas merged commit 619f395 into wagtail:main Feb 10, 2023
@lb-
Copy link
Member

lb- commented Feb 10, 2023

Thanks Thibaud

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

8 participants