feat(rules): rules can be enabled/disabled

[andrewazores]

Fixes #525

[tthvo]

Look great to me!

[lkonno]

It looks good to me as well.

[andrewazores]

Rebased and added a small commit that uses the WebSocket notification channel to update the rules table to reflect the rules' updated state, rather than refreshing the whole table with a GET query.

[tthvo]

Looks good :D Probably, we should a test to check the UI when receiving the new RuleUpdated notifications?

[andrewazores]

I'm having a hard time getting that test to work. Within the testing context it seems like the switch's styling isn't rendering how I expect it to, and in the DOM both the "on" and "off" states of the switch appear to be visible at the same time...

[tthvo]

I'm having a hard time getting that test to work. Within the testing context it seems like the switch's styling isn't rendering how I expect it to, and in the DOM both the "on" and "off" states of the switch appear to be visible at the same time...

Oh yess interesting! I guess we could find the span element with class pf-m-on/off and check toBeVisible?

[maxcao13]

Feels and looks good!

[andrewazores]

I'm having a hard time getting that test to work. Within the testing context it seems like the switch's styling isn't rendering how I expect it to, and in the DOM both the "on" and "off" states of the switch appear to be visible at the same time...

Oh yess interesting! I guess we could find the span element with class pf-m-on/off and check toBeVisible?

That's what I've been trying. I committed the current state of my test, but skipped so that it doesn't fail the build. At the end of 'update a rule when receiving a notification' I have these queries and expectations:

    const labels = container.querySelectorAll('label');
    expect(labels).toHaveLength(1);
    const label = labels[0];
    const spans = label.querySelectorAll(':scope > .pf-c-switch__label');
    expect(spans).toHaveLength(2);

    const onState = label.querySelector(':scope > .pf-c-switch__label.pf-m-on');
    expect(onState).toBeInTheDocument();
    expect(onState).not.toBeVisible();

    const offState = label.querySelector(':scope > .pf-c-switch__label.pf-m-off');
    expect(offState).toBeInTheDocument();
    expect(offState).toBeVisible();

but, both onState and offState are visible...

[andrewazores]

I guess those elements are hidden by the Patternfly css applying display: none or display: block to them. I don't think that stylesheet gets loaded in these tests though, does it? So as far as the test renderer can tell these things both are actually visible. Not sure what to do about that.

[tthvo]

I'm having a hard time getting that test to work. Within the testing context it seems like the switch's styling isn't rendering how I expect it to, and in the DOM both the "on" and "off" states of the switch appear to be visible at the same time...

Oh yess interesting! I guess we could find the span element with class pf-m-on/off and check toBeVisible?

That's what I've been trying. I committed the current state of my test, but skipped so that it doesn't fail the build. At the end of 'update a rule when receiving a notification' I have these queries and expectations:

    const labels = container.querySelectorAll('label');
    expect(labels).toHaveLength(1);
    const label = labels[0];
    const spans = label.querySelectorAll(':scope > .pf-c-switch__label');
    expect(spans).toHaveLength(2);

    const onState = label.querySelector(':scope > .pf-c-switch__label.pf-m-on');
    expect(onState).toBeInTheDocument();
    expect(onState).not.toBeVisible();

    const offState = label.querySelector(':scope > .pf-c-switch__label.pf-m-off');
    expect(offState).toBeInTheDocument();
    expect(offState).toBeVisible();

but, both onState and offState are visible...

Hmm, just looking back at the docs. How about we remove the label props to Switch (instead of setting it as "" - its treated as labeled so 2 spans are created)? This way we only have a single span within <label> to check?

[andrewazores]

Heh. Yea, the reason I set the label explicitly to the empty string is that if it isn't specified then the component falls back to displaying the aria-label visually. I don't want that label appearing inside a table row, though. I could do that if I remove the separate Name table column and use the first column to display both the Switch and the rule's name, but then the rule deleting/updating logic is broken because we access the rules by the rowData, and now we would not have any index in that array that contains just the unique rule name that we need for API calls - it would be a Switch component with the name as part of a label instead.

I could remove both the label and aria-label but then that's bad for accessibility and it logs an error in the console that at least one of those two should be defined.

[tthvo]

Right! Make sense! But I think probably there is an icon in-place if label is not set https://www.patternfly.org/v4/components/switch/#without-label?

Though, this would not solve the issue. I checked the element style in js-dom, but they don't have block: none as in the real browsers. Do you think it would be some js-dom limitation when loading css?

[andrewazores]

I think so. I don't think any CSS even gets loaded by our test setup. The real application hooks the css in in index.tsx, but the test harness just loads each component directly from its declaration and renders it, and there is no apparent link from that component definition to any kind of stylesheets at all.

[tthvo]

Just went throught some js-dom issue and saw this: testing-library/jest-dom#209 (comment). Could be the root issue?

I guess a hack for test would be adding an additional class in className (i.e. enabled/disabled-switch) of Switch, which does nothing but to test if the element has that according class?

[andrewazores]

That issue sounds related and perhaps would be a blocker for any fix on our end where we do try to load the PF CSS.

Anyway, hacking it by adding a specific className sounds like a good enough fix for now.

[tthvo]

Looks good :D