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

Template picker #13905

Closed
wants to merge 7 commits into from
Closed

Template picker #13905

wants to merge 7 commits into from

Conversation

Oetzie
Copy link
Contributor

@Oetzie Oetzie commented May 18, 2018

What does it do?

The new template picker feature. Before you create a new resource you will get a window where you can choose the 'pagetitle' and preview the 'template' (if screenshot is available). This PR also includes the new 'parent field combo' and 'template field combo'.

@Oetzie Oetzie changed the base branch from 2.x to 3.x May 18, 2018 11:32
@Jako
Copy link
Collaborator

Jako commented May 19, 2018

Nice solution! But could you remove the css changes from the PR and mark it with 'build required'. The scss changes are enough and don't lead to large merge conflicts.

@wax100
Copy link
Contributor

wax100 commented Jun 4, 2018

@Oetzie
If I upload printscreen of Full page, I receive cropped image.
e069208c-ff4e-4476-8670-10117bc8c579

screenshot_2
Is it possible to add magnific popup ?
Or something like http://verstaemvse.ru/demo/zoom/ ?

@bezumkin
Copy link
Contributor

bezumkin commented Jun 5, 2018

You need to rebase this PR with current 3.x branch.

Right now there a lot of old code, for example you can't upload image in the root of media source because of old media sources. This will stop many people from test it.

@bezumkin bezumkin added this to the v3.0.0-alpha milestone Jun 5, 2018
Copy link
Collaborator

@Mark-H Mark-H left a comment

Choose a reason for hiding this comment

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

Few small tweaks needed, but overall this looks like a solid improvement that especially end-users should find useful.

if ($this->modx->hasPermission('new_document')) {
$record = '{context_key: \"' . $this->modx->getOption('default_context') . '\", parent: 0}';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably safer to turn this into a json_encode to ensure it's valid JSON.

if ($this->modx->hasPermission('new_weblink')) {
$record = '{class_key: \"modWebLink\", context_key: \"' . $this->modx->getOption('default_context') . '\", parent: 0}';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as above

if ($this->modx->hasPermission('new_symlink')) {
$record = '{class_key: \"modSymLink\", context_key: \"' . $this->modx->getOption('default_context') . '\", parent: 0}';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as above

if ($this->modx->hasPermission('new_static_resource')) {
$record = '{class_key: \"modSymLink\", context_key: \"' . $this->modx->getOption('default_context') . '\", parent: 0}';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as above

fn : function(data, data2) {
console.log('failure');
console.log(data);
console.log(data2);
Copy link
Collaborator

Choose a reason for hiding this comment

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

These console.log statements should be replaced with "proper" error handling. That could be as simple as an alert showing the failure reason or a MODx.Msg

'</tpl>', {
group : null,
label : null,
getGroup : function(label, time) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Help me out here - why is this including the time? What's that for?

typeAhead : true,
tpl : new Ext.XTemplate('<tpl for=".">' +
'<tpl if="!Ext.isEmpty(this.getGroup(values.category_name, values.time))">' +
'<div class="x-combo-list-group">{this.label}</div>' +
Copy link
Collaborator

Choose a reason for hiding this comment

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

As the label appears to be coming from a remote source , please add htmlEncode as filter to it to prevent persistent XSS.

typeAhead : true,
tpl : new Ext.XTemplate('<tpl for=".">' +
'<tpl if="!Ext.isEmpty(this.getGroup(values.context_name, values.time))">' +
'<div class="x-combo-list-group">{this.label}</div>' +
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as earlier, please add htmlEncode to this.label to prevent potential XSS.

@Mark-H Mark-H added the blocked Active participation around the pull request or issue required. Consensus is not reached. label Nov 2, 2018
@Oetzie
Copy link
Contributor Author

Oetzie commented Jan 15, 2019

Sorry for late response, I will try to fix the feedback from @Mark-H thursday

@JoshuaLuckers
Copy link
Contributor

Let us know if we can help you @Oetzie

@Ibochkarev
Copy link
Collaborator

@Oetzie Is there time to finish the job you have started?

@Ibochkarev
Copy link
Collaborator

@Oetzie ping...

@Mark-H
Copy link
Collaborator

Mark-H commented Sep 1, 2019

@gpsietzema I know this is something Sterc was really excited about, but it's conflicting massively against latest 3.x, and there has not been movement in 9 months on any of the requested changes.

If you're still excited about this feature, can you please work it into your internal planning to keep this moving? For now I'm going to close the PR, but I hope to see a fresh one opened again soon. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked Active participation around the pull request or issue required. Consensus is not reached.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants