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

Use SVG icon instead of font icon in modeladmin index view #7562

Merged
merged 1 commit into from Oct 10, 2021

Conversation

jeromelebleu
Copy link
Contributor

@jeromelebleu jeromelebleu commented Oct 1, 2021

This uses the same classes and tags than wagtailadmin/shared/header.html to construct the <header> element and its children.

Contribute to #6107.

@squash-labs
Copy link

squash-labs bot commented Oct 1, 2021

Manage this branch in Squash

Test this branch here: https://jeromelebleufeatmodeladmin-ind-b64hj.squash.io

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.

@jeromelebleu just one comment for now about editing the changelog, it is blocking the CI runner.

I see there is a larger discussion about icons via #7565 so will let that resolve before looking into this further.

However, as per that discussion I noticed that the fa icons did not work with this change but the built in SVG icons did.

Another idea might be to try to leverage the header shared include even more, although it does require a change to that include.

See patch
idea-modeladmin-header.patch.txt

CHANGELOG.txt Outdated Show resolved Hide resolved
@jeromelebleu
Copy link
Contributor Author

Thanks for your comments @lb-!

I see there is a larger discussion about icons via #7565 so will let that resolve before looking into this further.

I think this PR can be treated separately as it does not target the same components.

However, as per that discussion I noticed that the fa icons did not work with this change but the built in SVG icons did.

I don't use fa icons but I understood that wagtail-font-awesome-svg should work, is it the case?

Another idea might be to try to leverage the header shared include even more, although it does require a change to that include.

I don't really see the goal of your proposal; this new template - modeladmin/includes/header_with_search.html - will be included only once, almost everything from wagtailadmin/shared/header.html is overwritten, and modeladmin/index.html already provides some blocks which could permit to extend just some part of it. Could you tell more?

@lb-
Copy link
Member

lb- commented Oct 4, 2021

@jeromelebleu thanks for the response above, just ignore that patch suggestion, my thinking was it would be good to leverage the shared includes for all headers but that is not really the purpose of this PR.

@lb- lb- requested a review from zerolab October 4, 2021 10:04
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.

There is a minor (.2px) discrepancy between the final h1 size, but with this version the icon and the heading are better centred next to each other

- This uses the same classes and tags than 'wagtailadmin/shared/header.html'
- constructs the <header> element and its children and blocks to that convention
@lb- lb- force-pushed the feat/modeladmin-index-svg-icon branch from 4cc4704 to 2c239cb Compare October 10, 2021 03:11
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.

@jeromelebleu thanks for your work on this and responding to feedback. @zerolab I have taken a look and think this looks good. As per discussion on other items I think we are now all in with SVG icons.

I have updated this branch with release notes, rebased on master and will merge in.

@lb- lb- merged commit a714de8 into wagtail:main Oct 10, 2021
@jeromelebleu jeromelebleu deleted the feat/modeladmin-index-svg-icon branch October 13, 2021 10:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants