Storage: show skeletons only when needed

[lslezak]

Problem

• The loading skeletons are displayed always, even when not needed
• The installation device selection popup is empty when clicked quickly before loading all data

Solution

Loading Skeletons

• Pass the name of the component which is changed by user to parent
• Send back the changing component to all children
• There is mapping which defined whether a child needs refresh when some setting is being changed

Installation Device Progress

• Use the sent data when when the stored values are undefined
• Show a simple skeleton

screenreaderText Properties

• I have removed all screenreaderText properties for skeletons and keep it only for the result section
• The reason is that it does not make sense to notify users about 5 values in progress, it usually takes about 2 seconds to refresh the data so the screen reader would not have enough time to read all messages
• Use it only the result section which is refreshed always, moreover the data for the other values will be read together with the result data, they will always take the same time

Questions

• Do we need more granularity with changing values? I.e. if the Btrfs snapshots are enabled/disabled then the partition list below is refresh as well. (IMHO fine to solve this later in a separate card / PR.)
• Better progress for the installation device loading? (Note: do not overengineer here, the progress is displayed only when you click the link very quickly after opening the storage page, in most cases it won't be used at all.)

Testing

• Tested manually

Screenshots

When enabling/disabling the Btrfs snapshots all data was refreshed, now the installation device and the space policy does not show the progress skeletons.

Original	Fixed

Another fix was empty installation device screen when the "Installation device" link was clicked quickly, before loading the data finished.

Original	Fixed

[coveralls]

coverage: 75.022% (-0.03%) from 75.053%
when pulling 2f954fa on storage_skeletons
into 493706b on master.

[ancorgs]

I see the form for selecting the installation device is now protected from "quick clickers"[1].

But that's the only pop-up that got that protection. What about these?

• Encryption - if the current settings have encryption enabled but I click on "Encryption" before data is loaded, then I get a form that makes me think encryption is not enabled. I expected a "loading" message/spinner/something while the data loads and then getting the form correctly initialized.
• Find space - if I click before the data is loaded, I get a Javascript error "policy is undefined".

[1] Although I don't like using a skeleton within the pop-up instead of some kind of "spinner", but that's another topic for another comment.

[ancorgs]

Honestly, this skeleton here looks really weird.

As commented during the planning meeting of the card and again today during review, the goal of the skeletons is to "reserve space" in the UI for the future content, so things don't jump up and down as that content is loaded into its place.

In case of a pop-up that is going to load at once as soon the content arrives, that multi-line skeleton looks like some kind of render error. I expected the pop-up to open kind of blank but with some "loading" message and/or a spinner (I personally expected the same three circles we have when loading the whole UI).

[ancorgs]

Shouldn't this include CHANGING.POLICY? I don't see how changing the find space policy can affect the list of volumes.

[ancorgs]

Just a comment about this. Being fully strict, the fact is that the content of the sentence displayed right after "Find Space" can actually be affected by changes in the target disk(s), in the booting disk and in the list of volumes.

For example, the sentence can initially be "deleting all content of the installation disk". But then I change the target and decide to install on an LVM over sdb and sdc. Then the sentence changes to "deleting all content of the 2 selected disks". Then I decide that, in addition to that, the system will boot from sda so the sentence changes to "deleting all content of the 3 selected disks". And for extra fun I decide to allocate /home as a new partition at sdd. Then the sentence changes again to "deleting all content of the 4 selected disks".

I don't know if those changes in the sentence really deserve a skeleton. I just wanted to mention there is SOME influence and as such it should be documented next to the code.

[ancorgs]

In fact, BootConfigField is never affected by changes in other settings. It only makes sense to display it as a skeleton when the whole page is loading. But once the value is set it can only be changed via its very own "Change boot options" button.

Watching it become a skeleton so often feels quite wrong.

[lslezak]

This allows lazy evaluation of the branches. The problem is that <If> evaluates both branches and then returns one of them.

So for example this

<If condition={data === undefined} then={<Loading/>} else={<Foo foo={data.foo}>} />

crashes when data is undefined because it evaluates the else branch too early.

With this improvement you can pass a function which can be evaluated only when the result is needed. (Passing functions is quite common in Javascript to allow the lazy evaluation.)

Then the fix is trivial, just add () => before the value:

<If condition={data === undefined} then={<Loading/>} else={() => <Foo foo={data.foo}>} />

@dgdavid are you OK with this change?

I'll add some example into the <If> documentation if you agree with this improvement.

[dgdavid]

Thanks @lslezak

Let's say that I have no strong opinion about it. At first sight, it looks reasonable to prevent not needed evaluations. But on the other hand it's weird to me to accept a function as content of the branches. IMHO it should be limited to ReactNode (Although technically a component is a function too).

That said, I fully understand the proposal looking at the code written at https://github.com/openSUSE/agama/pull/1171/files#diff-994f1f4ff00fecec1797a2aaa799dbbeddc4e8fb2c1ae666fedbb95b7c499770R155-R178

  <If
        condition={isLoading}
        then={
          <Loading text={_("Loading data...")} />
        }
        else={
          () =>
            <Form id="space-policy-form" onSubmit={onSubmit}>
              <SpacePolicyPicker currentPolicy={policy} onChange={setPolicy} />
              <If
                condition={devices.length > 0}
                then={
                  <SpaceActionsTable
                    devices={devices}
                    expandedDevices={expandedDevices}
                    deviceAction={deviceAction}
                    isActionDisabled={policy.id !== "custom"}
                    onActionChange={changeActions}
                  />
                }
              />
            </Form>
        }
      />

But I would go for a different implementation instead. For me, what is in the else should be a component that React should be able to understand. Let's say <SpacePolicyForm />. Then, the component should be aware about what it needs for return either, an output or nothing.

const SpacePolicyForm = ({ prop1, prop2 }) => {
  if (!prop1 || !prop2) return;

  ...

  return (
    <div><b>{prop1}:</b>{prop2}</div>
  );
}

I.e., the key for me is to write more robust components.

Of course, I might be wrong, so maybe another view here could help to decide what way to go.

[joseivanlopez]

I was assuming the else branch is not evaluated if the condition is true. So, probably, I have written unsafe code.

[dgdavid]

BTW, as far as I can see I think that this set of changes reveals another change we could address: to add the isLoading prop to core/Popup and make it show the spinner or its children based on the prop value. That way, we could reduce the <If condition={isLoading} then={... repetition and have more clean WhateverDialog.jsx components.

[lslezak]

After reading this I decided to revert the <If> change. It seems it is not safe and it can have some nasty side effects in some cases.

[imobachgs]

@joseivanlopez Actually I do not think you have written "unsafe" code. The Form itself is not executed (@dgdavid and me has done some tests with another example). The problem is the data.foo when data is undefined (that's the code that gets executed).

[dgdavid]

LGTM, thanks.

[dgdavid]

NP: We use to mock the PF/Skeleton and look for the mocked content instead, which is somehow more aligned with RTL principles (to look for what the user see, not how it's styled).

https://github.com/openSUSE/agama/blob/7e43d5883ef894000194c841c2e547ca3fa81455/web/src/components/storage/ProposalPage.test.jsx#L40-L48

But feel free to keep the test as it is.

[dgdavid]

It's also more robust in the sense that no matter if in the next PF update they change the HTML node for wrapping the content or the CSS classes used for styling it. The latest will happens as soon as we migrate to PFv6.