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

Improvement #79

Merged
merged 2 commits into from
Apr 28, 2019
Merged

Improvement #79

merged 2 commits into from
Apr 28, 2019

Conversation

wilsonge
Copy link

@wilsonge wilsonge commented Apr 27, 2019

Removes code and prevents the double control-group stuff.

However comes with different issues on the permissions fields:

  1. Your PR is missing if there's more than 1 field in the permissions fieldset.
  2. Also in your current PR (but more unlikely practical use case) we've removed the ability to have a showon based on the permissions field (it technically existed before)

My PR has the issue that it fixes that but effectively reintroduces the controls class which means the permissions field is shifted left. So I gave it it's own class revert-permissions and overriding the margin-left. Not convinced it's amazing but it works :/

@brianteeman
Copy link
Owner

Not convinced it's amazing but it works :/

That was my view of my own code

Your PR is missing if there's more than 1 field in the permissions fieldset.

Can you give me an example of where we have that

@wilsonge
Copy link
Author

Nothing I'm aware of in core. It seems relatively unlikely because we'd always be hiding the label. But before we did handle it and now we don't

@brianteeman brianteeman merged commit 1eaae4b into brianteeman:com_config Apr 28, 2019
@brianteeman
Copy link
Owner

Thanks @wilsonge

@wilsonge wilsonge deleted the config_fields branch April 28, 2019 15:56
brianteeman pushed a commit that referenced this pull request May 26, 2020
* Add codeowners

* Rename transition tab options to actions
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.

2 participants