ENH Pre-render the LinkField in entwine #241

maxime-rainville · 2024-03-01T08:52:30Z

This PR is making the pre-rendered UI more pleasing by pre-rendering somethings.

Before

no-pre-load-linkfield-2024-03-01_21.55.02.webm

Before React boots

Before LinkField gets its data back

After

pre-load-linkfield-2024-03-01_21.53.59.webm

Before React boots

Before LinkField gets its data back

Parent issue

Pre-render linkfields to provide a nicer UI until React has time to boot #242

Depends on

ENH Move spinner to a seperate template so other module can include it silverstripe-admin#1705

maxime-rainville · 2024-03-01T09:25:12Z

client/src/components/LinkField/LinkField.js

-      if (!linkData) {
+      // Render dataless item to provide a good loading experience, except if we have single link field
+      const linkData = data[linkID] || {};
+      if (!linkData && !isMulti) {
        continue;


By not skipping the link entries for which we don't have data yet, we'll render a list of placeholder LinkPickerTitle. This will avoids having a big jump on the page once the data finally gets back to us.

maxime-rainville · 2024-03-01T09:28:32Z

client/src/components/LinkField/LinkField.js

@@ -446,7 +446,7 @@ const LinkField = ({

  const saveRecordFirst = !loadingError && ownerID === 0;
  const renderLoadingError = loadingError;
-  const renderPicker = !loadingError && !inHistoryViewer && !saveRecordFirst && (isMulti || Object.keys(data).length === 0);


This was needed because a disabled LinkField with pe-existing data would briefly render a picker and a place holder link.

With this change, it only renders the picker.

maxime-rainville · 2024-03-01T09:29:31Z

src/Form/MultiLinkField.php

+
+    public function LinkIDs(): ArrayList
+    {
+        return ArrayList::create($this->dataValue());


This wraps the IDs in an ArrayList so the SS template can loop over them.

templates/SilverStripe/LinkField/Form/LinkField_Spinner.ss

GuySartorelli

Are the new templates always used for rendering those elements, or does react render them separately?
If the latter, this gives the false impression that you can change how the linkfield gets rendered by changing templates.

maxime-rainville · 2024-03-03T20:51:46Z

Enwtine only.

Most of these templates were pre-existing, so you could easily have been confused into thinking that you could override them. If you did try to customise them, you would quickly notice that your changes a being nuked once React boots.

I can rename the templates and/or add extra comments to make it clear this is an Entwine only construct.

GuySartorelli · 2024-03-03T22:10:28Z

Enwtine only.

Linkfield is a react component, so that doesn't really make any sense

Most of these templates were pre-existing, so you could easily have been confused into thinking that you could override them. If you did try to customise them, you would quickly notice that your changes a being nuked once React boots.

The existing ones CAN be overridden, but notably are used to bootstrap the react component.

The new templates mirror something that is rendered by react - so changing these templates will only change the display of a linkfield before any react is bootstrapped for the field if I'm not mistaken.

At the very very least these templates need comments at the top indicating that they are only for use in rendering the field before react has bootstrapped and will not affect the display of the rendered react component.

maxime-rainville · 2024-03-03T22:24:31Z

Behat build is broken. I thought it was intermittent, but it seems to be a legit failure.

I'll add comments to the template while I'm at it.

tests/behat/features/accessibility-linkfield.feature

emteknetnz

We shouldn't be adding templates to a react component

tests/behat/features/accessibility-linkfield.feature

templates/SilverStripe/LinkField/Form/LinkField_Spinner.ss

client/src/components/LinkField/tests/LinkField-test.js

maxime-rainville

I've addressed the feedback.

On the broader point of "adding templates to a react component", providing placeholder markup while waiting for some long running process to complete is a standard practice in most modern JS based application.

For example, here's what ZenHub does.

It provides a nicer UI for the user, avoid things jumping around on the page once the long process has completed and reassures the user that the site hasn't crash on them.

I appreciate we have not historically done this. Now is as good a time to start as any.

templates/SilverStripe/LinkField/Form/LinkField_Spinner.ss

tests/behat/features/accessibility-linkfield.feature

client/src/components/LinkField/tests/LinkField-test.js

maxime-rainville · 2024-03-07T01:00:23Z

client/src/components/LinkField/tests/LinkField-test.js

+  }) });
+  await screen.findByText('Page title');
+  const linkPicker = container.querySelectorAll('#link-picker__link-123');
+  expect(linkPicker[0]).toHaveAttribute('aria-disabled', 'false');


There's an aria-disabled attribute on the Disabled linkfield ... but its value is false. I'm not sure if that's actually what we want to do here.

I've added like this for now. We can revisit in the accessibility card.

emteknetnz · 2024-03-07T01:41:44Z

client/src/components/LinkField/tests/LinkField-test.js

+  }) });
+  await screen.findByText('Page title');
+  const linkPicker = container.querySelectorAll('#link-picker__link-123');
+  expect(linkPicker[0]).toHaveAttribute('aria-disabled', 'false');


Suggested change

expect(linkPicker[0]).toHaveAttribute('aria-disabled', 'false');

Adding it here make it look like this is the correct behaviour, even though we know it's wrong. If we're explicitly adding aria-disabled on the other card then we'll naturally add a unit test for it

Removed the test for false.

tests/behat/features/accessibility-linkfield.feature

emteknetnz

There's little orange 'modified' badges showing before it's loaded as part of the 'loading animation' which shouldn't be there

This video showing me refreshing the page on my local

linkfield-fout-after-2024-03-07_15.25.41.webm

I can't help thinking we need to look at the spinner styling, the dark background on it make for a pretty jarring experience on my local where there's lots of "flashing" because the background is rendered for so little time. There already was some before this PR, though it's quite a lot worse afterwards

templates/SilverStripe/LinkField/Form/LinkField_Spinner.ss

maxime-rainville

Added the missing comment and cleaned the version state icon.

FYI this PR now depends on silverstripe/silverstripe-admin#1705

maxime-rainville · 2024-03-09T10:37:06Z

client/src/components/LinkField/tests/LinkField-test.js

+  }) });
+  await screen.findByText('Page title');
+  const linkPicker = container.querySelectorAll('#link-picker__link-123');
+  expect(linkPicker[0]).toHaveAttribute('aria-disabled', 'false');


Removed the test for false.

maxime-rainville · 2024-03-09T10:38:37Z

client/src/constants/versionStates.js

@@ -0,0 +1,9 @@
+const versionStates = {


Repeating the same strings over and over again seemed wrong and likely to lead to confusion. So I moved them to a constant.

Could you please refrain from doing random refactoring half-way through a peer review. It adds unnecessary burden at the end of the person doing the peer review.

maxime-rainville · 2024-03-09T10:40:18Z

client/src/components/LinkPicker/LinkPickerTitle.js

@@ -198,4 +201,8 @@ LinkPickerTitle.propTypes = {
  buttonRef: PropTypes.object.isRequired,
 };

+LinkPickerTitle.defaultProps = {
+  versionState: versionStates.unversioned,


This change addresses the problem where the place holder LinkPickerTitle would get a draft state icon.

maxime-rainville · 2024-03-09T10:41:04Z

composer.json

@@ -17,7 +17,8 @@
        "php": "^8.1",
        "silverstripe/framework": "^5.2",
        "silverstripe/cms": "^5",
-        "silverstripe/versioned": "^2"
+        "silverstripe/versioned": "^2",
+        "silverstripe/admin": "^2.2"


We have a dependency on admin 2.2 because of the new spinner template.

templates/SilverStripe/LinkField/Form/LinkField_Spinner.ss

emteknetnz

Tested locally

emteknetnz

Behat failures in CI

maxime-rainville · 2024-03-12T05:31:47Z

Merging the admin PR fixed the behat build.

maxime-rainville commented Mar 1, 2024

View reviewed changes

maxime-rainville mentioned this pull request Mar 1, 2024

Pre-render linkfields to provide a nicer UI until React has time to boot #242

Closed

maxime-rainville marked this pull request as ready for review March 1, 2024 09:44

GuySartorelli requested changes Mar 2, 2024

View reviewed changes

maxime-rainville self-assigned this Mar 3, 2024

maxime-rainville force-pushed the pulls/4/improve-pre-loaded-state branch from 12ac60a to 61ee9fe Compare March 5, 2024 02:18

maxime-rainville changed the base branch from 4 to 4.0 March 5, 2024 02:26

ENH Pre-render the LinkField in entwine

8a891cd

maxime-rainville force-pushed the pulls/4/improve-pre-loaded-state branch from 61ee9fe to 8a891cd Compare March 5, 2024 09:28

maxime-rainville commented Mar 6, 2024

View reviewed changes

tests/behat/features/accessibility-linkfield.feature Show resolved Hide resolved

emteknetnz requested changes Mar 6, 2024

View reviewed changes

tests/behat/features/accessibility-linkfield.feature Show resolved Hide resolved

templates/SilverStripe/LinkField/Form/LinkField_Spinner.ss Outdated Show resolved Hide resolved

client/src/components/LinkField/tests/LinkField-test.js Show resolved Hide resolved

MNT Correct disabled link Jest test

281a934

maxime-rainville commented Mar 7, 2024

View reviewed changes

maxime-rainville assigned emteknetnz and unassigned maxime-rainville Mar 7, 2024

emteknetnz reviewed Mar 7, 2024

View reviewed changes

emteknetnz requested changes Mar 7, 2024

View reviewed changes

GuySartorelli reviewed Mar 7, 2024

View reviewed changes

templates/SilverStripe/LinkField/Form/LinkField_Spinner.ss Show resolved Hide resolved

maxime-rainville assigned maxime-rainville and unassigned emteknetnz Mar 9, 2024

maxime-rainville added 3 commits March 9, 2024 23:28

ENH Move versioned states to a constant

8d0b81f

ENH Use global Spinner template

684dcc5

MNT Revert test change for aria-disabled

26d7050

maxime-rainville commented Mar 9, 2024

View reviewed changes

emteknetnz approved these changes Mar 10, 2024

View reviewed changes

emteknetnz requested changes Mar 10, 2024

View reviewed changes

emteknetnz approved these changes Mar 12, 2024

View reviewed changes

emteknetnz merged commit 967c457 into silverstripe:4.0 Mar 12, 2024
12 checks passed

emteknetnz deleted the pulls/4/improve-pre-loaded-state branch March 12, 2024 20:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH Pre-render the LinkField in entwine #241

ENH Pre-render the LinkField in entwine #241

maxime-rainville commented Mar 1, 2024 •

edited

maxime-rainville Mar 1, 2024

maxime-rainville Mar 1, 2024

maxime-rainville Mar 1, 2024

GuySartorelli left a comment

maxime-rainville commented Mar 3, 2024

GuySartorelli commented Mar 3, 2024

maxime-rainville commented Mar 3, 2024

emteknetnz left a comment

maxime-rainville left a comment

maxime-rainville Mar 7, 2024

emteknetnz Mar 7, 2024

maxime-rainville Mar 9, 2024

emteknetnz left a comment •

edited

maxime-rainville left a comment

maxime-rainville Mar 9, 2024

maxime-rainville Mar 9, 2024

emteknetnz Mar 10, 2024

maxime-rainville Mar 9, 2024

maxime-rainville Mar 9, 2024

emteknetnz left a comment

emteknetnz left a comment

maxime-rainville commented Mar 12, 2024

ENH Pre-render the LinkField in entwine #241

ENH Pre-render the LinkField in entwine #241

Conversation

maxime-rainville commented Mar 1, 2024 • edited

Before

Before React boots

Before LinkField gets its data back

After

Before React boots

Before LinkField gets its data back

Parent issue

Depends on

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuySartorelli left a comment

Choose a reason for hiding this comment

maxime-rainville commented Mar 3, 2024

GuySartorelli commented Mar 3, 2024

maxime-rainville commented Mar 3, 2024

emteknetnz left a comment

Choose a reason for hiding this comment

maxime-rainville left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emteknetnz left a comment • edited

Choose a reason for hiding this comment

maxime-rainville left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emteknetnz left a comment

Choose a reason for hiding this comment

emteknetnz left a comment

Choose a reason for hiding this comment

maxime-rainville commented Mar 12, 2024

maxime-rainville commented Mar 1, 2024 •

edited

emteknetnz left a comment •

edited