[WIP] Added PF4 namespace to data-ouia-component-type attributes #3622

quarckster · 2020-01-30T14:45:50Z

In order to distinguish similar components from different libraries data-ouia-component-type attribute should have namespaces.
Fixes #3626

patternfly-build · 2020-01-30T14:54:11Z

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

codecov-io · 2020-01-30T14:58:13Z

Codecov Report

Merging #3622 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #3622   +/-   ##
=======================================
  Coverage   67.15%   67.15%           
=======================================
  Files         907      907           
  Lines       25559    25559           
  Branches     2267     2267           
=======================================
  Hits        17165    17165           
  Misses       7353     7353           
  Partials     1041     1041

Flag	Coverage Δ
#misc	`95.45% <ø> (ø)`	⬆️
#patternfly3	`69.29% <ø> (ø)`	⬆️
#patternfly4	`64.31% <ø> (ø)`	⬆️
#patternfly4integration	`?`

Impacted Files	Coverage Δ
...nfly-4/react-core/src/components/Select/Select.tsx	`64.28% <ø> (ø)`	⬆️
...rnfly-4/react-table/src/components/Table/Table.tsx	`91.35% <ø> (ø)`	⬆️
...nfly-4/react-core/src/components/Button/Button.tsx	`88.46% <ø> (ø)`	⬆️
...eact-core/src/components/Breadcrumb/Breadcrumb.tsx	`75% <ø> (ø)`	⬆️
...nfly-4/react-core/src/components/Switch/Switch.tsx	`66.66% <ø> (ø)`	⬆️
...patternfly-4/react-core/src/components/Nav/Nav.tsx	`76.59% <ø> (ø)`	⬆️
...fly-4/react-core/src/components/ChipGroup/Chip.tsx	`78.84% <ø> (ø)`	⬆️
...ernfly-4/react-core/src/components/Alert/Alert.tsx	`84.21% <ø> (ø)`	⬆️
...re/src/components/Dropdown/DropdownWithContext.tsx	`90.47% <ø> (ø)`	⬆️
...tternfly-4/react-core/src/components/Tabs/Tabs.tsx	`69.35% <ø> (ø)`	⬆️
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fbcb1da...23276c0. Read the comment docs.

dlabrecq · 2020-01-30T16:31:07Z

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

      aria-label={ariaLabel}
      {...(ouiaContext.isOuia && {
-        'data-ouia-component-type': 'Alert',
+        'data-ouia-component-type': 'PF4/Alert',


Probably want an issue for this to discuss the format. For example, we may want to omit the version and use something more like pf-alert?

I agree with @dlabrecq . Is there a formal spec for this?

@dlabrecq What if a someday new version of Patternflywill be released let's say PF5 and some application will have to use both of them?

@redallen There is a spec https://ouia.readthedocs.io/en/latest/README.html#ouia-component:

A root level HTML element with a data-ouia-component-type attribute describing a unique name identifying ALL HTML components that can be controlled with the same code or interactions. These identifiers MAY and SHOULD be namespaced when used within a framework, particularly when the name is generic. The delimiter between namespace and type name should be a single / character.

e.g. A page that has a special dropdown could choose to name that dropdown as FrameworkA/CustomDropdown. All instances of this FrameworkA/CustomDropdown component MUST be expected to be able to be controlled via the same automation.

We don't have this for our CSS selector names, so thought to ask. I'm fine with whatever the team decides here. Just thinking the team should discuss...

redallen · 2020-01-30T16:41:56Z

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

      aria-label={ariaLabel}
      {...(ouiaContext.isOuia && {
-        'data-ouia-component-type': 'Alert',
+        'data-ouia-component-type': 'PF4/Alert',


I agree with @dlabrecq . Is there a formal spec for this?

quarckster · 2020-02-03T15:33:21Z

Updated the PR after discussion with @karelhala and @psav. Namespace should be defined in a separate attribute and be always enabled.

packages/patternfly-4/react-core/src/components/Dropdown/DropdownWithContext.tsx

quarckster · 2020-02-04T08:31:53Z

@karelhala @redallen, please review

dlabrecq · 2020-02-04T14:36:20Z

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

      aria-label={ariaLabel}
      {...(ouiaContext.isOuia && {
        'data-ouia-component-type': 'Alert',
+        'data-ouia-component-ns': 'PF4',


Could this be defined in one place, instead of within each component?

@dlabrecq it's a good point but my knowledge of typescript is limited, I believe it can be done somewhere here https://github.com/patternfly/patternfly-react/blob/master/packages/patternfly-4/react-core/src/components/withOuia/withOuia.tsx

Thinking we just need to import a common file. Perhaps ouia.ts or withOuia.tsx would be a good location to store the namespace?

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

Any chance we can use a qualified URI as the namespace, instead of a basic string? Something like https://www.patternfly.org/v4 as the namespace is more semantic and readable.

@psav @karelhala ^

Here's a good article in case it is helpful

https://www.w3.org/2001/tag/doc/URNsAndRegistries-50-2006-08-17.html#args_http

redallen · 2020-02-04T15:10:38Z

I think the need to add a namespace has revealed our OUIA implementation has too much boilerplate. We should not be hardcoding the string 'PF4' for each OUIA component. We'll discuss the best way to implement this, which might be doing away with the withOUIA HOC.

redallen · 2020-02-05T18:54:30Z

Hey @karelhala , @quarckster , and @psav , our infrastructure team discussed our OUIA implementation today and we would like to ignore localstorage and always include OUIA attributes using a helper function like:

export function getOUIAProps(componentName, id) {
  return {
        'data-ouia-component-ns': 'PF4',
        'data-ouia-component-type': componentName,
        'data-ouia-id': id
  };
}

and then in components:

import { getOUIAProps } from '../../helpers/getOUIAProps';
export class Alert extends React.Component {
  render() {
    return <div {...getOUIAProps(Alert.displayName, props.ouiaID)} />
  }
}

However, we would like to time this with our breaking change release since it changes the way components are rendered in snapshots. Is this a pressing issue, or can we wait until our breaking change release? Who do we expect to modify the current outstanding PRs after this change?

cc @dgutride @evwilkin

quarckster · 2020-02-11T08:19:57Z

@redallen Let's wait for the breaking change release. After that this PR should be closed and new issue and PR should be created. I will take over it.

redallen · 2020-03-18T16:39:57Z

Closing in favor of #3927.

Added PF4 namespace to data-ouia-component-type attributes

751260d

quarckster requested review from jschuler and karelhala January 30, 2020 14:54

dlabrecq reviewed Jan 30, 2020

View reviewed changes

quarckster mentioned this pull request Jan 30, 2020

feat(TextInput): Added OUIA for TextInput #3174

Closed

redallen previously approved these changes Jan 30, 2020

View reviewed changes

quarckster dismissed redallen’s stale review via 30ccb2a February 3, 2020 15:30

karelhala suggested changes Feb 3, 2020

View reviewed changes

packages/patternfly-4/react-core/src/components/Dropdown/DropdownWithContext.tsx Outdated Show resolved Hide resolved

quarckster force-pushed the ouia_namespace branch 2 times, most recently from 8eb5975 to 234d7b9 Compare February 3, 2020 16:02

Added data-ouia-component-ns

23276c0

quarckster force-pushed the ouia_namespace branch from 234d7b9 to 23276c0 Compare February 3, 2020 16:04

karelhala approved these changes Feb 4, 2020

View reviewed changes

dlabrecq requested changes Feb 4, 2020

View reviewed changes

quarckster changed the title ~~Added PF4 namespace to data-ouia-component-type attributes~~ [WIP] Added PF4 namespace to data-ouia-component-type attributes Feb 11, 2020

redallen mentioned this pull request Mar 4, 2020

Integration with typescript appears to be broken when using latest deps #3824

Closed

This was referenced Mar 18, 2020

perf(ouia): make ouia functional, add namespace #3927

Merged

Add namespacing into data-ouia-component-type attributes #3626

Closed

redallen closed this Mar 18, 2020

[WIP] Added PF4 namespace to data-ouia-component-type attributes #3622

[WIP] Added PF4 namespace to data-ouia-component-type attributes #3622

Uh oh!

Conversation

quarckster commented Jan 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

patternfly-build commented Jan 30, 2020

Uh oh!

codecov-io commented Jan 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

quarckster Jan 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dlabrecq Jan 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

quarckster commented Feb 3, 2020

Uh oh!

Uh oh!

quarckster commented Feb 4, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

redallen commented Feb 4, 2020

Uh oh!

redallen commented Feb 5, 2020

Uh oh!

quarckster commented Feb 11, 2020

Uh oh!

redallen commented Mar 18, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

quarckster commented Jan 30, 2020 •

edited

Loading

codecov-io commented Jan 30, 2020 •

edited

Loading

quarckster Jan 30, 2020 •

edited

Loading

dlabrecq Jan 30, 2020 •

edited

Loading