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

Refactoring com_fields to no longer use com_categories #13019

Merged
merged 11 commits into from
Dec 5, 2016

Conversation

Bakual
Copy link
Contributor

@Bakual Bakual commented Nov 25, 2016

This is a bigger refactoring of com_fields to get rid of the use of com_categories for its field groups (fieldset) feature.

During the rewrite I found also several code pieces which are unused or were copy-pasted from com_categories but don't make sense in the context of fields.
I also replaced the useage of some deprecated methods.

So forgive me that it become a bit bigger than initially planned 😄

Summary of Changes

  • Main thing is that the field groups are now handled in com_fields using own views.
  • ACL should work now at least better than before.

Testing Instructions

  • Make sure managing the fields and groups works
  • Also test that the fields appear as expected. You don't have to test the indivual field types as I haven't changed anything there.
  • Please also test ACL. It should inherit permissions from component to field group to field. I haven't had the time yet to test that myself but in theory the code should work.

Documentation Changes Required

If there already is documentation how to implement com_fields into an extension, that will need to be adjusted.

  • Needs a helper method which returns possible contexts.
  • Needs added permission sections in the access.xml file.

@Bakual Bakual added this to the Joomla 3.7.0 milestone Nov 25, 2016
*
* @return boolean
*
* @since 1.6
Copy link
Contributor

Choose a reason for hiding this comment

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

DEPLOY_VERSION
??

(Just to prove i read the code ;) )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will fix. Thanks!

'type', 'a.type',
'alias', 'a.alias',
'state', 'a.state',
'access', 'a.access', 'access_level',
Copy link
Contributor

Choose a reason for hiding this comment

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

elsewhere you have put access_level on its own row in the code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't really matter, but for consistency (since I like that myself as well) I will change it.

@brianteeman
Copy link
Contributor

Quick comment on fields in com_contact
There is a dropdown (as before) but it is showing more than it should - see screenshot
image

I dont believe it is supposed to be showing articles and users?

Also by default you open with contact as the context and a url of

administrator/index.php?option=com_fields&context=com_contact.contact

when you switch the context to mail in the select you get a url of

administrator/index.php?option=com_fields&view=fields

I expected

administrator/index.php?option=com_fields&context=com_contact.mail

@brianteeman
Copy link
Contributor

Looking closer
I see this dropdown everywhere now. Before you only had it on the two context in mail

That in itself is NOT a problem
the problem is that when you switch context to a different component you dont really notice as the only thing that is changing on the screen is the title which is ABOVE and to the left of the select. From a UI perspective this is outside of your field of vision and its confusing

@Bakual
Copy link
Contributor Author

Bakual commented Nov 25, 2016

Actually that is intented behaviour, but something which can be discussed if we want it or not.
The dropdown would allow to switch between the extensions that support custom fields. It takes the contexts from the extensions helper method. The sidebar automatically adjusts to the selected extension.

The URL doesn't need the context/extension anymore after it is initially set. So if you click the link in the sidebar, you see the extension/context in the URL. It will then set the userstate and from there on it's not needed anymore. If you use the dropdown, the extension/context will be sent in the form (POST) and not in the URL (GET). So that is fine.

@Bakual
Copy link
Contributor Author

Bakual commented Nov 25, 2016

the problem is that when you switch context to a different component you dont really notice as the only thing that is changing on the screen is the title which is ABOVE and to the left of the select. From a UI perspective this is outside of your field of vision and its confusing

The sidebar also changes, but I agree it may not be noticed.

@brianteeman
Copy link
Contributor

It is very confusing - it will lead to bug reports. It also prevents someone from direct linking to a field menu (unless you understand the secret urls)

fields

@Bakual
Copy link
Contributor Author

Bakual commented Nov 25, 2016

It also prevents someone from direct linking to a field menu (unless you understand the secret urls)

Direct links should work imho. The sidebar and menu links doen't do anything else and they work fine as far as I tested.
Not going to argue about the confusing part. I'm not good with UIs anyway.
I just kind of liked the possibility to switch fast between the extension and thought I add it. It's simple to remove again if we don't want it. We just have to adjust the formfield.
It would also be simple to adjust the options eg to "Contact -> Mail" instead of just "Mail" if that helps.

@brianteeman
Copy link
Contributor

Direct links should work imho.

they will if you copy the one that has the full context in. They wont if you use the url you see when you use the switcher as seen in the video

@Bakual
Copy link
Contributor Author

Bakual commented Nov 25, 2016

Yep, that's true. Not sure how important it is. It may be a bit tricky to do and likely involves JavaScript if you want to change the URL from a select. I think in current staging it's done using JS.

@laoneo
Copy link
Member

laoneo commented Nov 26, 2016

IMO it will be confusing for the end user to have all of the sudden an option to switch between components. In Joomla we don't have such functionality nowhere, so I would not introduce it. I would also do it the way that when there is only one context in the component, that the field will be hidden at all.

@Bakual
Copy link
Contributor Author

Bakual commented Nov 26, 2016

In Joomla we don't have such functionality nowhere, so I would not introduce it.

To be fair, the only place where we currently have a similar cross-extension extension is com_categories. That one doesn't have such a select, that's true.
But that's not really an argument to me since we're only talking about one possible place.
The Google project for translating/associating would be another such extension and that one could use such a selector.

You could look at it as a required option to filter fields. Like the search filters but not optional.
But if all think it is to confusing I have no problem removing it (or make it an option - which brings up another topic fast g)

@brianteeman
Copy link
Contributor

brianteeman commented Nov 26, 2016 via email

@laoneo
Copy link
Member

laoneo commented Nov 26, 2016

I can't find a use case where you are working on custom fields for articles and then you need to switch to the custom fields of users.

@Bakual
Copy link
Contributor Author

Bakual commented Nov 26, 2016

I can think of use cases when you're setting up a site.
But please this PR isn't about this single select (I already said I can remove it if all don't like it).
Much more important would be to test the function of the PR. Eg that fields and groups still work as expected and also ACL which should now work properly with inheritance from component -> groups -> fields.

@Bakual
Copy link
Contributor Author

Bakual commented Nov 26, 2016

Removed the switcher from groups view and only show it in fields view when there is more than one context.

@ralain
Copy link
Contributor

ralain commented Nov 27, 2016

Some issues emptying the trash of field groups:

  1. ACL error.
  2. Page title broken and sidebar empty.
  3. Context missing from URL, simply ending in "extension=". I assume this is the cause of 1 and 2.
  4. The success message states that 1 field was deleted (it was a field group.)

screen shot 2016-11-27 at 17 45 49

@ralain
Copy link
Contributor

ralain commented Nov 27, 2016

Also wrong context com_content.article on attempting to create a field under com_users.

@Bakual
Copy link
Contributor Author

Bakual commented Nov 27, 2016

@ralain Thanks for the test, the first one is indeed because there is an empty "&extension=" in the URL. I have to see where that comes from. The other is strange.

@Bakual
Copy link
Contributor Author

Bakual commented Nov 27, 2016

@ralain I think I have found the culprits. Can you please test again?

@ralain
Copy link
Contributor

ralain commented Nov 28, 2016

Yep, I am now able to create fields under com_users, and apart from the "field deleted" message, the rest appears to be fixed as well.

@Bakual
Copy link
Contributor Author

Bakual commented Nov 28, 2016

Fixed the message as well.
Also found another wrong redirect which is now fixed.

@tonypartridge
Copy link
Contributor

I have tested this item ✅ successfully on c4becd9

Great work @Bakual all working great now and messages are now spot on to me.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/13019.

@ralain
Copy link
Contributor

ralain commented Dec 4, 2016

I have tested this item ✅ successfully on c4becd9


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/13019.

@jeckodevelopment
Copy link
Member

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/13019.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Dec 4, 2016
@rdeutz rdeutz merged commit d086a4a into joomla:staging Dec 5, 2016
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Dec 5, 2016
@Bakual Bakual deleted the FieldsGroups branch December 5, 2016 09:17
@coolcat-creations
Copy link
Contributor

@Bakual Thank you for leading me to this PR. IMO i think such approach is better and more logical (and extendable later). Any thoughts?
image

@mbabker
Copy link
Contributor

mbabker commented Jan 24, 2017

Isn't there an open issue somewhere for this?

While I'm here, I'd say the way it is now is fine. It's also consistent with how categories are handled for extensions supporting those. Or are you saying you'd rather those all moved in favor of a categories item too?

Fields is integrated on a per-extension basis. For me that means I'd look for it with the extension.

@coolcat-creations
Copy link
Contributor

coolcat-creations commented Jan 24, 2017

For me it clutters the menu and doesn´t leave space for future improvements. Like that it would be isolated as an own area to build custom forms for any extension that supports it...

It´s more tidy, no?

@mbabker
Copy link
Contributor

mbabker commented Jan 24, 2017

You might be able to pull it off with your own custom admin menu. But the way core handles things, it's too complex to do that by default.

I honestly see the number of top level menus as clutter and adding one more makes it worse.

@Bakual
Copy link
Contributor Author

Bakual commented Jan 24, 2017

This should be discussed in an own issue, not in a merged PR. I just linked it because it was discussed here already before the current state got merged.

adding one more makes it worse.

The issue arose because there is another PR now which may raise those field links in the sidebar from currently two to three. It's not one single link like for categories.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet