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

Display context name and key in Context dropdown #13839

Merged
merged 1 commit into from Mar 29, 2018

Conversation

Projects
None yet
5 participants
@opengeek
Member

opengeek commented Mar 27, 2018

NOTE: this pull request requires a grunt build to be run to update minified js

What does it do?

Changed the context/getlist processor to add a display_name attribute comprised of the name and the key of the context which is then used in MODx.combo.Context to clarify the list of contexts presented to a user in various places in the manager UI.

Why is it needed?

This is to make it clearer when working with areas where the context key is being displayed in a grid, as in when editing context ACLs, and to provide a fallback for contexts where no name is provided.

Related issue(s)/PR(s)

Addresses #13739
Alternative to #13765

@opengeek opengeek added this to the v2.6.2 milestone Mar 27, 2018

@opengeek

This comment has been minimized.

Member

opengeek commented Mar 27, 2018

An example of the rendering of the combo in the default_context system setting:

system_settings___modx_revolution

@Oetzie

This comment has been minimized.

Contributor

Oetzie commented Mar 27, 2018

@opengeek remove the HTML from the processor and create a tpl ifor the combo box in ExtJS, or even better remove display_name from the processor and fix it in the tpl... This is a dirty fix..

And what is the point to show the context key if a name is available?

@opengeek

This comment has been minimized.

Member

opengeek commented Mar 27, 2018

@Oetzie I have no idea how to do that, and that was explained in the PR description.

@opengeek

This comment has been minimized.

Member

opengeek commented Mar 27, 2018

@Oetzie do you have an example of how I would create this tpl for the combo box?

@Oetzie

This comment has been minimized.

Contributor

Oetzie commented Mar 27, 2018

@opengeek You can use the following code in the context combo box. This tpl will show the name if it is set otherwise it will show the key name

,tpl: new Ext.XTemplate('<tpl for=".">' +
    '<div class="x-combo-list-item">' +
        '<tpl if="name">{name}</tpl>' + 
        '<tpl if="!name">{key}</tpl>' + 
    '</div>' +
'</tpl>')

Or you can use the following code to show the name and the key (BUT WHY?)

,tpl: new Ext.XTemplate('<tpl for=".">' +
     '<div class="x-combo-list-item">' +
        '{name} <span style="font-style: italic">({key})</span>' +
    '</div>' +
'</tpl>')
@Mark-H

This comment has been minimized.

Collaborator

Mark-H commented Mar 27, 2018

Jason does ExtJS :o Are you okay? Do we need to send help? :P

It is kinda potato/potato, the end result would be the same, and (ignoring programming language/template syntax differences) pretty much the same code, so I personally don't feel too strongly about using either approach. My gut says the js would be a better place as that is already generating the markup for the select box, but it boils down to preference mostly, I think.

Here's an example of the custom tpl approach

@opengeek

This comment has been minimized.

Member

opengeek commented Mar 27, 2018

ROFL @Mark-H! I think I just threw up a little in mouth looking at that XTemplate code.

@Oetzie

This comment has been minimized.

Contributor

Oetzie commented Mar 27, 2018

@opengeek The MODx Extjs Layer of MODX is a fucked up mess (bad coding, inline CSS wtf, dirty fixes)... I WOULD shame myself If I wrote a code like that... ;)

I would love to see Extjs 6.5 in MODx with a complete rebuild of the Extjs Layer because Extjs is freaking awesome.

@Mark-H

This comment has been minimized.

Collaborator

Mark-H commented Mar 27, 2018

@opengeek Yeah, ExtJS templating is lovely syntax. I've never dared to look into its internals. But you're mixing HTML with PHP inside a ternary, so, let's call that even ;)

@Oetzie's second example (edited in after my previous reply) looks good. The question why the key should be shown is a valid one - I do have one dev site where multiple contexts have the same name (which is a bad idea and not something I'd recommend), but it'd be useful there. And the PR description mentions grids that show a key but not the name, so that makes sense.

@Oetzie no need to get personal about previous technical decisions. Argue the code is terrible all you want, but let's try to keep judgment about the person and the code separated please. ;)

@Oetzie

This comment has been minimized.

Contributor

Oetzie commented Mar 27, 2018

@Mark-H the context key is just for table relations and stuff, not for displaying to a 'customer'. Just choose the right names for the context :-) Maybe its a good idea to make a user setting or permission to hide or show the context key for a customer.

Okay I will correct myself.. I WOULD shame myself If I wrote a code like that.. Better? :-)

Display context name and key in Context dropdown
This is to make it clearer when working with areas where the context key is being displayed in a grid, as in when editing context ACLs, and to provide a fallback for contexts where no name is provided.

Addresses #13739
@opengeek

This comment has been minimized.

Member

opengeek commented Mar 27, 2018

@Mark-H agreed—though I still think PHP + HTML is prettier. Plus I was using semantic markup… (LOL)

Pushed the solution using just the change to the combo using an XTemplate.

@Jako

This comment has been minimized.

Collaborator

Jako commented Mar 27, 2018

Templating is way better than adding a new column in the processor. I did not remembered that.

@opengeek

This comment has been minimized.

Member

opengeek commented Mar 27, 2018

The only problem with this approach is I had to switch to using key for displayName for the selected item in the combo or it would be blank if a name is not provided. This actually is more consistent with its usage in system settings (for default_context) anyway, but may not be the best solution overall. Without providing a solution from the processor, I'm at a loss how to address that part of the equation.

@Jako

This comment has been minimized.

Collaborator

Jako commented Mar 27, 2018

If a name is not provided, the value could be null and then it is not returned by the processor. But I did not check that.

@OptimusCrime

This comment has been minimized.

Contributor

OptimusCrime commented Mar 28, 2018

I am not 100% sure where this combo box is visible, but I like showing both key and name. If we provide the option to give a context a name, why not show it? Additionally, the key should also (always) be there, as this is the value we have to deal with in code. I do not see the problem with this PR at all.

@Oetzie

This comment has been minimized.

Contributor

Oetzie commented Mar 28, 2018

@opengeek ah I forgot that, but the name is leading and when you you displayField to key the key will be leading. In that case we need tocreate a new column in the processor and use that column as displayField...

$contextArray['display_name'] = $object->get('name');

if (empty($object->get('name'))) {
    $contextArray['display_name'] = $object->get('key');
}

The key should not be shown, we don't have to deal with the key on that level because the key will be put in a hidden textfield... Remember MODX 3 will be build for UI and user friendly, so less is more. The key showing in the combo box is not required. If you need the key as programmer (a customer/content editor wont need the context key) you can find it in the context grid.

@OptimusCrime ehm think harder.. In ACL you have the context combo box. Or in custom extra's like ClientConfig, in almost all of my extra's I use the context combo box.

@opengeek

This comment has been minimized.

Member

opengeek commented Mar 28, 2018

@Oetzie The key will be shown. And your justification is wrong. The filter in ACL context combo box is filtering a field that shows, wait for it… the key. And the xtype in the usage within system settings displays the value, which is again, the key. So let's stop with the personal jabs like "think harder" and stick to discussing the issue. I made the decision that it was better to display the key when selected. The name is there when selection is being made.

@Oetzie

This comment has been minimized.

Contributor

Oetzie commented Mar 28, 2018

@opengeek So you prefer multiple different styles for context instead of 1. Thats why the UI of MODx does not get improved allot...

For example:
Left resource tree: The context NAME is used...
Combo box: The context NAME is used in the list...
Combo box: The context KEY is used as display field...
ACL grid: The context KEY is used...

At one place the context key is used, and at other the place context name is used.. Yes thats very user friendly, not... The key is used for table relations and stuff, the name for in the UI...

@opengeek opengeek merged commit 1174bed into modxcms:2.6.x Mar 29, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment