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.

Sign up for GitHub

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

Jump to bottom

convert Language*Form to form builder #5252

Closed
wants to merge 2 commits into from
Closed

convert Language*Form to form builder #5252

wants to merge 2 commits into from

Conversation

mutec
Copy link
Contributor

@mutec mutec commented Jan 29, 2023

No description provided.

TimWolla
TimWolla suggested changes Jan 30, 2023
View reviewed changes
Copy link
Member

@TimWolla TimWolla left a comment

Choose a reason for hiding this comment

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

Thank you. Some remarks with regard to style, but this looks good to me in general.

wcfsetup/install/files/lib/acp/form/LanguageAddForm.class.php Outdated Show resolved Hide resolved
wcfsetup/install/files/lib/acp/form/LanguageAddForm.class.php Outdated Show resolved Hide resolved
wcfsetup/install/files/lib/acp/form/LanguageAddForm.class.php Outdated Show resolved Hide resolved
wcfsetup/install/files/lib/acp/form/LanguageAddForm.class.php Outdated Show resolved Hide resolved
wcfsetup/install/files/lib/acp/form/LanguageAddForm.class.php Outdated Show resolved Hide resolved
wcfsetup/install/files/lib/acp/form/LanguageAddForm.class.php Outdated Show resolved Hide resolved
wcfsetup/install/files/lib/acp/form/LanguageAddForm.class.php Outdated Show resolved Hide resolved
wcfsetup/install/files/lib/acp/form/LanguageEditForm.class.php Outdated Show resolved Hide resolved
@TimWolla
Copy link
Member

Looking at #5251: objectActionClass and objectEditLinkController are not defined. Did you test this?

@TimWolla TimWolla mentioned this pull request Jan 30, 2023
Closed
@mutec
Copy link
Contributor Author

mutec commented Jan 30, 2023

Looking at #5251: objectActionClass and objectEditLinkController are not defined. Did you test this?

I changed something after testing, maybe that part got lost. 😳

TimWolla
TimWolla suggested changes Jan 31, 2023
View reviewed changes
Copy link
Member

@TimWolla TimWolla left a comment

Choose a reason for hiding this comment

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

Now also did a functional test.

wcfsetup/install/files/lib/acp/form/LanguageEditForm.class.php Show resolved Hide resolved
wcfsetup/install/files/lib/acp/form/LanguageAddForm.class.php Outdated Show resolved Hide resolved
wcfsetup/install/files/lib/acp/form/LanguageAddForm.class.php Outdated Show resolved Hide resolved
wcfsetup/install/files/lib/acp/form/LanguageAddForm.class.php Outdated Show resolved Hide resolved
wcfsetup/install/files/lib/acp/form/LanguageAddForm.class.php Outdated Show resolved Hide resolved
wcfsetup/install/files/lib/acp/form/LanguageAddForm.class.php Show resolved Hide resolved
@TimWolla
Copy link
Member

TimWolla commented Feb 1, 2023

If the three open points:

  • Validate locale against languageCode
  • Remove ->required() for locale.
  • Fix the id validation.

are resolved and no other changes are made, then this can be merged. The other open-ended stuff (SingleSelectionField + requirement handling) can and should be resolved in a follow-up PR.

@TimWolla TimWolla mentioned this pull request Feb 1, 2023
Closed
@TimWolla
Copy link
Member

TimWolla commented Feb 6, 2023

@mutec Can you add the locale validation (#5252 (comment)) to bring this across the finish line?

@TimWolla
Copy link
Member

@mutec ping

@mutec
Copy link
Contributor Author

mutec commented Feb 21, 2023

@TimWolla pong
I'm trying to manage this on Saturday or Sunday; I'm pretty busy at the moment.

@TimWolla
Copy link
Member

Thank you. I've just wanted to make sure you've seen this, because there was no feedback on this one (not even an emoji reaction), but you commented on the package.json one.

@mutec
Copy link
Contributor Author

mutec commented Mar 15, 2023

@TimWolla I just found a few minutes. I added the validation to the languageCode-field. The other issues should have been resolved already. Could you please have a look on the error-message?

@TimWolla
Copy link
Member

@mutec Thank you for taking the time. I'll have a look right now to not delay this further.

TimWolla
TimWolla suggested changes Mar 15, 2023
View reviewed changes
wcfsetup/install/lang/en.xml Outdated Show resolved Hide resolved
wcfsetup/install/lang/de.xml Outdated Show resolved Hide resolved
wcfsetup/install/files/lib/acp/form/LanguageAddForm.class.php Outdated Show resolved Hide resolved
wcfsetup/install/files/lib/system/language/event/CopyLanguageContent.php Outdated Show resolved Hide resolved
wcfsetup/install/files/lib/acp/form/LanguageAddForm.class.php Outdated Show resolved Hide resolved
@mutec mutec force-pushed the lafofob branch 2 times, most recently from 902dabc to 53dd5fc Compare March 15, 2023 19:16
mutec added 2 commits March 15, 2023 21:16
@mutec
convert Language*Form to form builder
ebdde3e
@mutec
implement CopyLanguageContent
fb23670 
This event is meant to give 3rd-party developers the possibility to copy i18n contents in every case contents should be copied to another language.
TimWolla
TimWolla suggested changes Mar 16, 2023
View reviewed changes
Copy link
Member

@TimWolla TimWolla left a comment

Choose a reason for hiding this comment

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

Thank you. I can do all three of the remarks in follow-up commits. Let me know if you would like me to make the changes.

wcfsetup/install/files/lib/acp/form/LanguageAddForm.class.php Show resolved Hide resolved
wcfsetup/install/files/lib/acp/form/LanguageAddForm.class.php Show resolved Hide resolved
wcfsetup/install/files/lib/system/language/event/CopyLanguageContent.class.php Show resolved Hide resolved
@TimWolla
Copy link
Member

@mutec Please see: #5252 (review)

@mutec
Copy link
Contributor Author

mutec commented Apr 25, 2023

@TimWolla sorry, I don't want to waste my time any more. Feel free to finalize that.

@mutec mutec closed this Apr 25, 2023
@TimWolla
Copy link
Member

Thank you for confirming, I'll reopen this so it stays on my dashboard until I get around to include these follow-ups.

@TimWolla TimWolla reopened this Apr 26, 2023
TimWolla pushed a commit that referenced this pull request Apr 26, 2023
@mutec @TimWolla
Convert Language*Form to form builder
e59c27a 
see #5252

[Tim: Performed some final adjustments]
TimWolla pushed a commit that referenced this pull request Apr 26, 2023
@mutec @TimWolla
Implement LanguageContentCopying event
f855889 
This event is meant to give 3rd-party developers the possibility to copy i18n
contents in every case contents should be copied to another language.

[Tim: Renamed the event to match the event naming scheme]

see #5252
@TimWolla TimWolla closed this in 1fda2fb Apr 26, 2023
@TimWolla
Copy link
Member

Now applied, thank you. I've noted in the commit messages that I made changes after the fact.

@TimWolla TimWolla mentioned this pull request May 2, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dtdesign dtdesign dtdesign left review comments

@TimWolla TimWolla TimWolla requested changes

Assignees
No one assigned
Labels
Feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mutec @TimWolla @dtdesign