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

Some v1.2 frontend fixes #3228

Merged
merged 4 commits into from
Dec 28, 2021
Merged

Some v1.2 frontend fixes #3228

merged 4 commits into from
Dec 28, 2021

Conversation

askvortsov1
Copy link
Sponsor Member

@askvortsov1 askvortsov1 commented Dec 27, 2021

If there are no search sources, HTML for the Search component won't be rendered, so trying to attach listeners to it will likely error.

In this PR, we don't attach such listeners/logic if there are no sources. We also stop asserting that sources is defined to help avoid other similar issues in the future.
Adding `clickOutsideDeactivates` seems to fix the issue, contrary to what the focus-trap documentation implies about it being unnecessary.
@davwheat
Copy link
Member

Re-running tests because they were taking 30+ mins to install deps 🤣

Copy link
Member

@dsevillamartin dsevillamartin left a comment

Choose a reason for hiding this comment

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

Search & input fixes tested working.

Updated title logic needs fixing & question about BC.

Comment on lines -631 to -635
# Translations in this namespace are used to format page meta titles.
meta_titles:
with_page_title: "{pageNumber, plural, =1 {{pageTitle} - {forumName}} other {{pageTitle}: Page # - {forumName}}}"
without_page_title: "{pageNumber, plural, =1 {{forumName}} other {Page # - {forumName}}}"

Copy link
Member

Choose a reason for hiding this comment

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

Is this not a breaking change for language packs?

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

Hmm that's a tough one. As per the definition in https://discuss.flarum.org/d/26242-draft-public-api-policy, our public API is defined in terms of the interfaces of the code, not necessarily of behavior. This will require language packs to update, but in the meantime it won't break forums using those language packs, so I don't see it as illegal.

Copy link
Member

Choose a reason for hiding this comment

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

It's fine, we introduced these in this cycle anyway, so it doesn't matter, it never made it to a release.

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

The argument is that even requiring translations means that extensions will need to add translations to avoid showing the 'core.lib' text. However, since that's easily addressable and doesn't break functionality, I think it's fine.

Copy link
Member

Choose a reason for hiding this comment

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

I do agree with that.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good, I figured it wasn't that big of a deal as it's not breaking.

js/src/common/Application.tsx Outdated Show resolved Hide resolved
This gives more flexibility for customization, and allows overriding title structure via translations / linguist.
We need to specify a unique key for each modal so that the modals are fully destroyed and recreated. For instance, this fixes the signup modal being empty with OAuth register flows.
Copy link
Member

@dsevillamartin dsevillamartin left a comment

Choose a reason for hiding this comment

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

Confirmed fix for app title & modal opening (specifically oauth with sign up modal) working

@askvortsov1 askvortsov1 merged commit cff6724 into master Dec 28, 2021
@askvortsov1 askvortsov1 deleted the as/v1.2_frontend_fixes branch December 28, 2021 01:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consecutive shows of the same modal should reinitialize the modal
4 participants