Skip to content
This repository has been archived by the owner on Mar 1, 2024. It is now read-only.

[UI-library] Expose JSS InsertionPoint in PixelStreamingApplicationStyle. #390

Merged
merged 2 commits into from
Oct 19, 2023

Conversation

timbotimbo
Copy link
Contributor

Relevant components:

  • Signalling server
  • Frontend library
  • Frontend UI library
  • Matchmaker
  • Platform scripts
  • SFU

Problem statement:

I'm embedding a Pixelstreaming component using the UI-Library in a single-page application made in Blazor.

The ui-library styling is added to the <head> tag of the site, which makes the style persist for the life of the application.
Because of this, other unrelated parts of the application might be affected by this styling once the UI-Library has been used once.
(This is caused by class names like form-control and form-group being shared by multiple frameworks.)

The JSS library being used under the hood already has functionality to specify where in the DOM the style gets added, the InsertionPoint parameter. This parameter fixes the issue as the styling can be injected into specific DOM nodes that can be removed when Pixelstreaming can be discarded.

You still need to be vigilant for class conflicts in styling when Pixelstreaming is active in the page, but this way we won't have to test the style of every other component of the app.

Solution

This PR exposes the existing InsertionPoint parameter of JSS in the PixelStreamingApplicationStyle constructor.
I've named it jssInsertionPoint to clarify that it applies to the JSS options.

Example usage

// example of specifying an insertionPoint for the styling

- const PixelStreamingApplicationStyles = new PixelStreamingApplicationStyle();
+ const PixelStreamingApplicationStyles = new PixelStreamingApplicationStyle({ 
+    jssInsertionPoint: document.getElementById("unreal-root")
+ });

  PixelStreamingApplicationStyles.applyStyleSheet();

The effect in a minimal HTML page:
DOM

Documentation

InsertionPoint is documented as a parameter of the jss.setup function and in the JSS setup instructions.

insertionPoint - string value of a DOM comment node which marks the start of sheets or a rendered DOM node. Sheets rendered by this Jss instance are inserted after this point sequentially.

I did not see in depth documentation of PixelStreamingApplicationStyle, thus I did not update any docs.

Test Plan and Compatibility

Compatibility

This is a nullable parameter that was already implicitly null in the current implementation. Not using it should not break compatibility with any existing code.
In a personal repo I'm using this on the 5.2 branch, and as far as I can tell this can be backported to 5.2 and 5.3.

Testing

I did not add any unit tests as I felt that this would almost default to testing the JSS library itself.

Use cases that I tried manually:

  • Supplying a HTML node using document.getElementById() works as expected.

  • Suppling a string 'custom-insertion-point' works with a <!-- custom-insertion-point --> comment inside the element. (see the 2nd docs link)

  • Entering an invalid parameter (a Number using JS) will not throw any errors and give the default behavior.

  • Entering an invalid string will show a clear warning in the browser console.
    error

Signed-off-by: timbotimbo <timbotimbo@users.noreply.github.com>
@timbotimbo timbotimbo changed the title Expose JSS InsertionPoint in PixelStreamingApplicationStyle. [UI-library] Expose JSS InsertionPoint in PixelStreamingApplicationStyle. Oct 18, 2023
@Belchy06 Belchy06 self-assigned this Oct 19, 2023
Copy link
Collaborator

@Belchy06 Belchy06 left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@lukehb lukehb left a comment

Choose a reason for hiding this comment

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

This looks safe and adds some good extensibility to the styling. Happy to have this in. Thanks!

@Belchy06 Belchy06 merged commit 8ba4101 into EpicGames:master Oct 19, 2023
github-actions bot pushed a commit that referenced this pull request Oct 19, 2023
Signed-off-by: timbotimbo <timbotimbo@users.noreply.github.com>
Co-authored-by: William Belcher <william.belcher@xa.epicgames.com>
(cherry picked from commit 8ba4101)
github-actions bot pushed a commit that referenced this pull request Oct 19, 2023
Signed-off-by: timbotimbo <timbotimbo@users.noreply.github.com>
Co-authored-by: William Belcher <william.belcher@xa.epicgames.com>
(cherry picked from commit 8ba4101)
github-actions bot pushed a commit that referenced this pull request Oct 19, 2023
Signed-off-by: timbotimbo <timbotimbo@users.noreply.github.com>
Co-authored-by: William Belcher <william.belcher@xa.epicgames.com>
(cherry picked from commit 8ba4101)
@github-actions
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
UE5.2
UE5.3
UE5.4

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation and see the Github Action logs for details

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

Successfully merging this pull request may close these issues.

3 participants