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

Customized icons for content types #14383

Merged
merged 13 commits into from Mar 1, 2019

Conversation

@Alroniks
Copy link
Collaborator

commented Feb 15, 2019

What does it do?

It adds to content types new field (column) with icon class that will be shown in the resource tree.

Also, during work on this, I've fixed one issue about updating grid when CheckColumn plugin is used. See details in 4a89afe

Why is it needed?

It will allow to easier determine that resource has a different content type.
NOTE: Template icon can override the content type icon as content type more basic entity than a template.

For HTML content type icon is set to empty to not change existing condition.

Small animation about how it works.

content-type-icons

Related issue(s)/PR(s)

#4841

@Alroniks Alroniks marked this pull request as ready for review Feb 18, 2019

@Alroniks Alroniks requested review from Mark-H and opengeek as code owners Feb 18, 2019

$iconCls[] = $classKeyIcon;
$contentType = $resource->getOne('ContentType');
if ($contentType && $contentType->get('icon')) {
$iconCls = [];

This comment has been minimized.

Copy link
@JoshuaLuckers

JoshuaLuckers Feb 19, 2019

Collaborator
Suggested change
$iconCls = [];
array_shift($iconCls);

This comment has been minimized.

Copy link
@Mark-H

Mark-H Feb 19, 2019

Collaborator

This line seems unnecessary entirely ($iconCls is already initialised empty in line 435) and I'm not sure what array_shift would accomplish on an empty array?

This comment has been minimized.

Copy link
@JoshuaLuckers

JoshuaLuckers Feb 20, 2019

Collaborator

I suggested it to make sure things won't break if someone adds a new if statement with the same kind of logic before this one.

This comment has been minimized.

Copy link
@Alroniks

Alroniks Feb 27, 2019

Author Collaborator

array_shift deletes only first element but the idea is to clear the whole array before setting new icons.

$template = $resource->getOne('Template');
if ($template && $template->get('icon')) {
$iconCls = [];

This comment has been minimized.

Copy link
@JoshuaLuckers

JoshuaLuckers Feb 19, 2019

Collaborator
Suggested change
$iconCls = [];
array_shift($iconCls);
$iconCls[] = $template->get('icon');
}
if (!count($iconCls)) {

This comment has been minimized.

Copy link
@JoshuaLuckers

JoshuaLuckers Feb 19, 2019

Collaborator
Suggested change
if (!count($iconCls)) {
if (empty($iconCls)) {

We don't need to know how large the array is only if it's empty.

@@ -137,6 +146,10 @@ Ext.extend(MODx.grid.ContentType,MODx.grid.Grid,{
});
window.show(e.target);
}

,renderIconField: function (v, md, rec) {
return new Ext.XTemplate('<i class="icon icon-lg {icon}"></i>&nbsp;&nbsp; {icon}').apply(rec.data);

This comment has been minimized.

Copy link
@Mark-H

Mark-H Feb 19, 2019

Collaborator

worth applying a htmlEncode to prevent introducing new XSS issues

@Jako Jako added this to the v3.0.0-alpha milestone Feb 21, 2019

@Ruslan-Aleev

This comment has been minimized.

Copy link
Contributor

commented Feb 26, 2019

I tested it, for some reason I don’t see an icon for "icon-json", see the picture below.
I forgot to rebuild styles, everything works well.

@Ruslan-Aleev
Copy link
Contributor

left a comment

Everything works as expected.

@Alroniks

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 27, 2019

@Mark-H @JoshuaLuckers I've improved the code a bit but without changing how it should work.

@Mark-H

Mark-H approved these changes Feb 28, 2019

@Alroniks Alroniks merged commit be637d3 into modxcms:3.x Mar 1, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

Alroniks added a commit that referenced this pull request Mar 1, 2019

Customized icons for content types #14383
* upstream/pr/14383:
  Some fixes after review: remove possible place for xss and adjust setting initial values for list of icons
  Fixed issue with icon priority
  Make content type icons working in the tree
  Removed duplicated styles
  Improved naming of content types
  Added icon to grid and create/update window
  Fixed error with faling grid updating when CheckColumn used
  Refactored create/update processors for content types
  Added icon for json
  Added icon to field list
  Added default values for icon field for content types
  Added new columns to map files and added migration
  Added icon field to content type object in schema

@Alroniks Alroniks deleted the Alroniks:feature-4841 branch Mar 1, 2019

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