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

Nested children support in overlays #521

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

joepie91
Copy link

This change makes it possible for translations to contain nested markup tags, like so:

foo = This is some <em><strong>nested markup!</strong></em>

It also fixes the test which explicitly checks that this doesn't work, because that test was broken and incorrectly still passed after making this change. ¯\_(ツ)_/¯ That's a separate commit though, so it can be picked out if desired.

Current state of tests: all tests pass, except the one which explicitly disallows this behaviour (obviously), as I'm not sure whether there's a specific reason why nested children have not been allowed up til now. I have not added new tests for this functionality, as I find it very frustrating to deal with TS and again, I have no idea whether this PR will be accepted or not :)

So yeah, this is a bit of a draft PR, I suppose. I need this in my own application (for basic Markdown support in translation strings), hence having created a fork, but I don't know whether you're also interested in it as the upstream.

@zbraniecki
Copy link
Collaborator

Thank you for that draft!

DOM Overlays are actually quite sophisticated as a system. I wouldn't want to change one piece without considering others.

We want to think about it this year, and here are features we want to think about - https://github.com/zbraniecki/fluent-domoverlays-js/wiki/New-Features-(rev-3)

In issues in that repo you can read about prototypes and architectural considerations around each feature.

Would you be interested in participating in such architecture design round for rev3? It will take a looong time tho :)

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

2 participants