Check if OUIA is set after mounting #2478

jschuler · 2019-07-08T20:16:00Z

Closes: #2477

Adding OUIA capabilities to library components

Import: import { InjectedOuiaProps, withOuiaContext } from '../withOuia';
For TS combine the props with the InjectedOuiaProps class Switch extends React.Component<SwitchProps & InjectedOuiaProps>
Wrap the component in the withOuiaContext higher-order-component

const  SwitchWithOuiaContext = withOuiaContext(Switch);
export { SwitchWithOuiaContext as Switch };

OUIA props are in this.props.ouiaContext

const { ouiaContext, ouiaId } = this.props;
<label
	className=""
	htmlFor=""
	{...ouiaContext.isOuia && {
	'data-ouia-component-type':  'Switch',
	'data-ouia-component-id':  ouiaId || ouiaContext.ouiaId
	}}
>my label</label>

Consumer usage

Case 1: non-ouia users

<Switch  />

No re-render, does not render ouia attributes

Case 2: enable ouia through local storage

in local storage ouia: true

<Switch  />

render's ouia attribute data-ouia-component-type="Switch"

Case 3: enable ouia through local storage and generate id

in local storage ouia: true

in local storage ouia-generate-id: true

<Switch  />

render's ouia attributes data-ouia-component-type="Switch" data-ouia-component-id="0"

Case 4: enable ouia through local storage and provide id

in local storage ouia: true

<Switch ouiaId="my_switch_id" />

render's ouia attributes data-ouia-component-type="Switch" data-ouia-component-id="my_switch_id"

Case 5: enable ouia through context and provide id

Note: If context provided isOuia is true and local storage provided isOuia is false, context will win out. Context will also win if its isOuia is false and local storage's is true. Context > local storage

import { OuiaContext } from  '@patternfly/react-core';
<OuiaContext.Provider value={{ isOuia: true }}>
	<Switch ouiaId="my_switch_id" />
</OuiaContext.Provider>

render's ouia attributes data-ouia-component-type="Switch" data-ouia-component-id="my_switch_id"

patternfly-build · 2019-07-08T20:25:12Z

PatternFly-React preview: https://patternfly-react-pr-2478.surge.sh

jschuler · 2019-07-08T22:31:17Z

See description on how to use in the library

karelhala

I wouldn't really change props of children, it is unsafe and if there is going to be an array of children it will break. Same for children wrapped with React.Fragment.

This is really perfect example for context. We can create context with default value and reuse it everywhere we want to access it. We can also export this context so consumers can use this as well, another benefit is that we could control the value by ourselves in future instead of using localStorage anybody else can use Provider and set the value there.

const OUIAContext = React.createContext({
  isOuia: isOUIAEnvironment()
});

<OUIAContext.Consumer>
  {({ isOuia }) => (
    <label
      //...
  </label>
  )}
</OUIAContext.Consumer>

jschuler · 2019-07-10T14:16:47Z

@karelhala Context uses render props. Usage would be the same, as it can be inserted at any level. Just like context, it needs a single child.
The advantage here is that the render props class packages additional functionality that makes it easy to reuse code and less boilerplate. For example it checks the localStorage after component mount as it wouldn't otherwise detect changes for statically built sites.

karelhala · 2019-07-10T14:50:06Z

@karelhala Context uses render props. Usage would be the same, as it can be inserted at any level. Just like context, it needs a single child.
The advantage here is that the render props class packages additional functionality that makes it easy to reuse code and less boilerplate. For example it checks the localStorage after component mount as it wouldn't otherwise detect changes for statically built sites.

I don't really like this approach of checking component render, setting state and rendering again because what you are essentially doing is rendering twice same component. And as we all know rendering is the most expensive operation. If it were used only on staticly rendered pages, I would be fine with that. But this is going to be used on live sites (with JS and window and such) and twice rendering the same elements will take a LOT of time.

How about we use Context as I proposed and add decorator as well that would set provider's value after mounting. We'd specifically say that this decorator is mostly for staticly rendered pages.

const withOUIA = WrappedComponent => {
   class ComponentWithOUIA extends React.Component {
      state: WithOUIAState = {
        renderWithOUIA: false,
        ouiaId: getOUIAUniqueId()
      };

      componentDidMount() {
        const { renderWithOUIA } = this.state;
        const isOuia = isOUIAEnvironment();
        if (isOuia !== renderWithOUIA) {
          this.setState({ renderWithOUIA: isOuia });
        }
      }

      render() {
        const { renderWithOUIA, ouiaId } = this.state;
        return <OUIAContext.Provider value={{ isOuia: renderWithOUIA, ouiaId }}>
          <WrappedComponent {...this.props} />
         </OUIAContext.Provider>
      }
  }
  return ComponentWithOUIA;
};

Hyperkid123 · 2019-07-10T14:58:28Z

@jschuler I agree with @karelhala.

Basically what i see here is a mix of of Context with some additional logic of component decorators (like withRouter for instance).

The problem of mixing these two is that you are forcing additional render after initial render (state or props mutation usually seen in decorators). This might be seen as a nit pick but i think we should try to be as efficient as possible. If you are leveraging client side rendering (and most of PF4 users are) you are left with subpar solution.

I would prefer having decorator for your static pages and context for client rendering. This would not even mean having duplicate code because you could use the context inside your decorator. And using the React Context comes with all the checks, optimizations etc.

jschuler · 2019-07-10T15:16:47Z

@karelhala @Hyperkid123 Can you explain what you mean by decorators? Also i am trying to understand how the context version differs all that much. Looks like it's a HOC instead of a render props component. About rendering twice, that would only happen if the local storage key 'ouia' is true, so for normal consumers this wouldn't change how often it gets rendered

redallen · 2019-07-10T15:49:41Z

@jschuler It doesn't force a rerender unless the prop is true, but React does have to crawl the tree again and stateless components with OUIA will have to become stateful, so this is a performance hit (although minor).

@Hyperkid123 Your suggested decorator is really just setting a flag when we build the docs, which is in #2487 and an alternative to this PR.

RonnyPfannschmidt · 2019-07-11T09:52:05Z

packages/patternfly-4/react-core/src/components/WithOUIA.tsx

@@ -0,0 +1,40 @@
+import * as React from 'react';
+import { isOUIAEnvironment, getUniqueId as getOUIAUniqueId } from '../helpers/ouia';


an absolutely critical note - the ouia id should identify an actual object via its uuid or pkey - having a incremented global as data source there is a massive problem

@jschuler @redallen @karelhala ^

@RonnyPfannschmidt Why is it a massive problem? It provides unique IDs every page load per-instance of the component that are consistent every page reload. If we were to generate a UUID instead with something like this solution, how could we make sure they're unique without a global data source that keeps a record of them all, is inconsistent between page loads, and has the same problem?

@redallen i believe you completely misunderstood the use for those ids - they are for identifying application object across potentially different pages, different views and different collections - you cannot generate them randomly or from a counter, you have to take database ids or primary keys

we are now iterating on the ouia spec as we noted that the component id specification is lacking a description of the intent to make this absolutely clear

@RonnyPfannschmidt We are a component library, have no database IDs or primary keys to map to components we create on the page. How do we comply to the spec *for our docs page at http://patternfly-react.surge.sh/?

@redallen thats why it is critical, a user can pass it in, the user will have the database keys,

for documentation pages/examples, exemplary ids are suitable

what's the format of these? haven't seen the spec yet. @redallen maybe something that can be passed in as a json string into another local var?

the id should be pretty much expressable as pain string

possible examples are 00000000-0000-0000-0000-000000000000, flammable, state-deffered, john@example.com

I wasn't sure about the auto generated IDs at first and given the spec we can't really generate them. If I understand correctly the spec these IDs are kinda like helpers to identify what is being rendered. So for instance if you have a list of cards on screen each card would have data-ouia-component-type="card" set, but the data-ouia-component-id would be probably UUID from database, or whatever to identify which data are actually rendered there.

I think that best way here would be to add uoiaId prop to every component that is marked by ouia spec. Consumers can then pass this prop to component and if ouia is enabled we will add it to root element. To ilustrate this for set of cards I will have a list of UUIDs from server and render user data in them

import React from 'react'; import { Card, CardBody } from '@patternfly/react-core'; const data = [{ uuid: '00000000-0000-0000-0000-000000000001', name: 'Foo' }, { uuid: '00000000-0000-0000-0000-000000000002', name: 'Bar' }]; export default () => (<React.Fragment> {data.map((user) => <Card key={user.id} ouiaId={user.id}> <CardBody>{user.name}</CardBody> </Card> )} </React.Fragment>)

The resulting HTML would look like

<div class="pf-c-card" data-ouia-component-id="00000000-0000-0000-0000-000000000001" data-ouia-component-type="Card"> <div class="pf-c-card__body"> Foo </div> </div> <div class="pf-c-card" data-ouia-component-id="00000000-0000-0000-0000-000000000002" data-ouia-component-type="Card"> <div class="pf-c-card__body"> Bar </div> </div>

RonnyPfannschmidt · 2019-07-12T09:47:00Z

Thanks for this excellent detail description

I believe it's often likely that render key and ouia ID can be the same, that may be a chance for convenience

jschuler · 2019-07-12T20:41:03Z

Ok, how about the new commit? I don't think we want to really advertise any OUIA props in our consumer docs but we can still allow the passing of a data-ouia-component-id prop for supported components. So for instance

<Switch
        id="simple-switch"
        label={isChecked ? 'Message when on' : 'Message when off'}
        isChecked={isChecked}
        onChange={this.handleChange}
        aria-label="Message when on"
        data-ouia-component-id="my-unique-data-id"
      />

Reading the spec, these ids are optional and can be left off unless passed in. If you wish to always add them (for example for the patternfly-react.surge.sh site) can add another local variable ouia-generate-id and set it to true.

redallen

This satisfies all the constraints.

karelhala · 2019-07-16T14:14:55Z

packages/patternfly-4/react-core/src/components/WithOUIA.tsx

+  render() {
+    const { renderWithOUIA, ouiaId } = this.state;
+    const { children } = this.props;
+    return children({


Is there any reason why are you using children instead of React context? I think that it is much easier approach and it allows for further improvements.

Okay I'll update to use context :)

redallen

Nitpicking. Nice work that should satisfy everyone and statically render correctly.

redallen · 2019-07-22T13:12:48Z

packages/patternfly-4/react-core/src/components/withOuia/withOuia.tsx

+  ouiaId?: number | string;
+}
+
+type Omit<T, K extends keyof T> = Pick<T, Exclude<keyof T, K>>;


How about instead import { Omit } from '../../helpers/typeUtils';?

redallen · 2019-07-22T13:14:48Z

packages/patternfly-4/react-core/src/components/Switch/Switch.tsx

+
+const SwitchWithOuiaContext = withOuiaContext(Switch);
+
+export { SwitchWithOuiaContext as Switch };


React-docgen is still unphased when grabbing the props for Switch. This is good.

karelhala

I really like the sage of context! just one correction PF shouldn't really take care of OUIA IDs because these will be bound to each application and each screen. Plus consumers should be able to have no ID if there is one component of same type present.

The simplest usage can be <Switch /> there can't be any ID because it's just one switch on screen.

PF should be aware of data-ouia-component-id and it should be part of rendered DOM only if ouia is enabled (either with localStorage or with context provider) however calculating it is not good. It is possible to pass it from context provider, however the final code will be janky and not good to read.

karelhala · 2019-07-22T18:46:35Z

packages/patternfly-4/react-core/src/components/withOuia/ouia.ts

+export const generateOUIAId = (): boolean => typeof window !== 'undefined' && window.localStorage['ouia-generate-id'] && window.localStorage['ouia-generate-id'].toLowerCase() === 'true' || false;
+
+let id = 0;
+export const getUniqueId = (): number => id++;


Patternfly shouldn't really provide these IDs because they will be bound to specific screen inside specific product. On one place the ID will represent inventory ID and on other it can represent for instance ordered products. And if there is going to be just one component of one type we can't have any ID. Plus autogenerated ID would break some UI automated tests where we compare rendered HTML.

id is only provided if ouia-generate-id in local storage is true. The main use case i see for this is the PatternFly docs, since our examples won't have manual ouia ids assigned to them. Otherwise no id is added to the component

karelhala · 2019-07-22T18:46:41Z

packages/patternfly-4/react-core/src/components/Switch/Switch.tsx

          'data-ouia-component-type': 'Switch',
-          'data-ouia-component-id': this.ouiaId
+          'data-ouia-component-id': ouiaContext.ouiaId


Can we remove ouiaId from context and consume it from props instead? It would be kinda repetetive to add id to multiple items trough context provider, plus not all items will have data-ouia-component-id set. Only if there will be multiple items of same component we will use ouiaId

ok, i'll make it so it can be consumed from props

tlabaj

LGTM

Comments addressed

jschuler · 2019-07-24T20:56:09Z

@karelhala I believe i've addressed your comments. If there is anything else I don't mind following up

patternfly-build · 2019-07-24T21:05:32Z

Your changes have been released in:

@patternfly/react-core@3.75.3
@patternfly/react-docs@4.9.3
@patternfly/react-inline-edit-extension@2.9.50
demo-app-ts@2.12.12
@patternfly/react-integration@2.12.1
@patternfly/react-table@2.14.24
@patternfly/react-topology@2.6.21
@patternfly/react-virtualized-extension@1.1.83

Thanks for your contribution! 🎉

Check if OUIA is set after mounting

9681586

jschuler requested review from tlabaj and redallen July 8, 2019 20:16

jschuler requested a review from dgutride July 8, 2019 20:29

reduce boilerplate by using renderprops

91515cd

jschuler added 3 commits July 9, 2019 09:31

snapshot

2eac1d3

snaps

bfc2b8b

snaps once more

da31275

redallen mentioned this pull request Jul 9, 2019

Added OUIA compatibility to Pagination component #2387

Closed

karelhala previously requested changes Jul 10, 2019

View reviewed changes

tlabaj assigned redallen Jul 10, 2019

tlabaj added PF4 React issues for the PF4 core effort chore 🏠 labels Jul 10, 2019

tlabaj requested a review from dlabaj July 10, 2019 17:22

redallen mentioned this pull request Jul 10, 2019

fix(ouia): add ouia to docs when statically rendering #2487

Closed

RonnyPfannschmidt reviewed Jul 11, 2019

View reviewed changes

allow user to pass in ouia id

6bb6d51

redallen previously approved these changes Jul 16, 2019

View reviewed changes

karelhala reviewed Jul 16, 2019

View reviewed changes

jschuler added 2 commits July 17, 2019 11:14

Merge remote-tracking branch 'upstream/master' into ouia-switch-attrs

8c6773c

ouia with context

7d34319

jschuler dismissed redallen’s stale review via 7d34319 July 17, 2019 15:44

see if moving into subdir makes build pass

40ecbf3

redallen suggested changes Jul 22, 2019

View reviewed changes

redallen previously approved these changes Jul 22, 2019

View reviewed changes

karelhala self-requested a review July 22, 2019 15:29

karelhala reviewed Jul 22, 2019

View reviewed changes

ouiaId as prop

5c8a374

jschuler dismissed redallen’s stale review via 5c8a374 July 24, 2019 20:26

revert examples

6921c41

redallen approved these changes Jul 24, 2019

View reviewed changes

tlabaj approved these changes Jul 24, 2019

View reviewed changes

tlabaj merged commit 8d396e7 into patternfly:master Jul 24, 2019

quarckster mentioned this pull request Sep 19, 2019

PF4: make common components OUIA compatible #2425

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check if OUIA is set after mounting #2478

Check if OUIA is set after mounting #2478

jschuler commented Jul 8, 2019 •

edited

patternfly-build commented Jul 8, 2019

jschuler commented Jul 8, 2019 •

edited

karelhala left a comment

jschuler commented Jul 10, 2019

karelhala commented Jul 10, 2019

Hyperkid123 commented Jul 10, 2019

jschuler commented Jul 10, 2019

redallen commented Jul 10, 2019

RonnyPfannschmidt Jul 11, 2019

quarckster Jul 11, 2019

redallen Jul 11, 2019

RonnyPfannschmidt Jul 11, 2019

RonnyPfannschmidt Jul 11, 2019

redallen Jul 11, 2019 •

edited

RonnyPfannschmidt Jul 11, 2019

jschuler Jul 11, 2019

RonnyPfannschmidt Jul 12, 2019

karelhala Jul 12, 2019

RonnyPfannschmidt commented Jul 12, 2019

jschuler commented Jul 12, 2019

redallen left a comment

karelhala Jul 16, 2019

jschuler Jul 16, 2019

redallen left a comment

redallen Jul 22, 2019

redallen Jul 22, 2019

karelhala left a comment

karelhala Jul 22, 2019

jschuler Jul 23, 2019

karelhala Jul 22, 2019

jschuler Jul 23, 2019

tlabaj left a comment

jschuler commented Jul 24, 2019

patternfly-build commented Jul 24, 2019

		@@ -0,0 +1,40 @@
		import * as React from 'react';
		import { isOUIAEnvironment, getUniqueId as getOUIAUniqueId } from '../helpers/ouia';


		const SwitchWithOuiaContext = withOuiaContext(Switch);

		export { SwitchWithOuiaContext as Switch };

Check if OUIA is set after mounting #2478

Check if OUIA is set after mounting #2478

Conversation

jschuler commented Jul 8, 2019 • edited

Adding OUIA capabilities to library components

Consumer usage

Case 1: non-ouia users

Case 2: enable ouia through local storage

in local storage ouia: true

Case 3: enable ouia through local storage and generate id

in local storage ouia: true

in local storage ouia-generate-id: true

Case 4: enable ouia through local storage and provide id

in local storage ouia: true

Case 5: enable ouia through context and provide id

Note: If context provided isOuia is true and local storage provided isOuia is false, context will win out. Context will also win if its isOuia is false and local storage's is true. Context > local storage

patternfly-build commented Jul 8, 2019

jschuler commented Jul 8, 2019 • edited

karelhala left a comment

Choose a reason for hiding this comment

jschuler commented Jul 10, 2019

karelhala commented Jul 10, 2019

Hyperkid123 commented Jul 10, 2019

jschuler commented Jul 10, 2019

redallen commented Jul 10, 2019

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

redallen Jul 11, 2019 • edited

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

RonnyPfannschmidt commented Jul 12, 2019

jschuler commented Jul 12, 2019

redallen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

redallen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karelhala 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

tlabaj left a comment

Choose a reason for hiding this comment

jschuler commented Jul 24, 2019

patternfly-build commented Jul 24, 2019

jschuler commented Jul 8, 2019 •

edited

jschuler commented Jul 8, 2019 •

edited

redallen Jul 11, 2019 •

edited