fix(accordion): adds prop to heading level of parent component #2273

jenny-s51 · 2019-06-14T14:46:25Z

What: closes #1881

jschuler · 2019-06-14T14:49:08Z

packages/patternfly-4/react-core/src/components/Accordion/AccordionToggle.js

@@ -3,21 +3,27 @@ import PropTypes from 'prop-types';
 import { css } from '@patternfly/react-styles';
 import { AngleRightIcon } from '@patternfly/react-icons';
 import styles from '@patternfly/react-styles/css/components/Accordion/accordion';
+import { AccordionContext } from './Accordion';
+import { ApplicationLauncherPosition } from '../ApplicationLauncher/applicationLauncherConstants';


this should not be here

Ah, the price of auto-import-on-tab-completion.

jschuler · 2019-06-14T14:49:52Z

packages/patternfly-4/react-core/src/components/Accordion/AccordionToggle.js

+          <button
+            id={id}
+            className={css(styles.accordionToggle, isExpanded && styles.modifiers.expanded, className)}
+            {...props}


would you mind making {...props} the last attribute

jschuler · 2019-06-14T14:50:49Z

packages/patternfly-4/react-core/src/components/Accordion/Accordion.js

-);
+export const AccordionContext = React.createContext({ headingLevel: 'h3' });
+
+export const HeadingLevelTypes = {


Why is this needed? I think we can just pass headingLevel through into context

Do you mean I should just set AccordionContext = React.createContext(); ?

I think this const is good to have for the consumer (it'll be the same as enums we export elsewhere, when this is ported over to the typescript version) but Joachim is saying that on line 18 we don't need const HeadingLevel = HeadingLevelTypes[headingLevel];, we can just pass lowercase-h headingLevel into the provider. The HeadingLevelTypes object doesn't really do anything for us internally, it's just for the consumer

patternfly-build · 2019-06-14T14:54:52Z

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

codecov-io · 2019-06-17T13:58:15Z

Codecov Report

❗ No coverage uploaded for pull request base (master@e7ef49d). Click here to learn what that means.
The diff coverage is 100%.

@@            Coverage Diff            @@
##             master    #2273   +/-   ##
=========================================
  Coverage          ?   80.63%           
=========================================
  Files             ?      666           
  Lines             ?     8431           
  Branches          ?      711           
=========================================
  Hits              ?     6798           
  Misses            ?     1280           
  Partials          ?      353

Flag	Coverage Δ
#patternfly3	`85.23% <ø> (?)`
#patternfly4	`76.22% <100%> (?)`
#patternflymisc	`95.79% <ø> (?)`

Impacted Files	Coverage Δ
...t-core/src/components/Accordion/AccordionToggle.js	`100% <100%> (ø)`
...4/react-core/src/components/Accordion/Accordion.js	`100% <100%> (ø)`

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 e7ef49d...c5aced0. Read the comment docs.

…nfly#2147) * Convert context selector to typescript * Fix linting and build error * Add demo and integration tests * Add typescript badge to docs * Made event argument optional

mturley

Looking really good! I think we probably also want to add a snapshot test to https://github.com/patternfly/patternfly-react/blob/master/packages/patternfly-4/react-core/src/components/Accordion/Accordion.test.tsx that renders an accordion with a non-default headingLevel prop, just to assert that it actually renders the alternate heading tag in the snapshot.

mturley · 2019-06-17T18:27:07Z

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

+  h6 = 'h6'
+}
+
+// declare global {


We can fully remove this now.

mturley · 2019-06-17T18:27:49Z

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

@@ -2,22 +2,44 @@ import * as React from 'react';
 import { css } from '@patternfly/react-styles';
 import styles from '@patternfly/react-styles/css/components/Accordion/accordion';

+export const AccordionContext = React.createContext('h3');
+
+export enum HeadingLevel {


Judging by our discussion in Slack, since the user already has TitleLevel we don't need this enum after all.

mturley · 2019-06-17T18:29:09Z

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

  ...props
 }: AccordionProps) => (
  <dl className={css(styles.accordion, className)} aria-label={ariaLabel} {...props}>
-    {children}
+    <AccordionContext.Provider value={ headingLevel }>{children}</AccordionContext.Provider>


I guess we still have eslint turned off, but I think when it's eventually turned back on it'll complain about the spacing here, I think you want to remove the spaces around headingLevel here (value={headingLevel})

Thank you! I resolved the changes you mentioned above in this commit here

mturley · 2019-06-17T19:35:04Z

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

@@ -1,6 +1,9 @@
 import * as React from 'react';
 import { css } from '@patternfly/react-styles';
 import styles from '@patternfly/react-styles/css/components/Accordion/accordion';
+import {TitleLevel} from '../Title';


You don't need to import this here, it's not being used in the component itself.

mturley · 2019-06-17T19:37:42Z

packages/patternfly-4/react-core/src/components/Accordion/AccordionToggle.tsx

-      </button>
-    </h3>
+    <AccordionContext.Consumer>
+      {(TitleLevel: any) => (


Sorry to be confusing here, but I think this is actually better off staying as HeadingLevel like it was before, since that's the name of the prop that is passed into Accordion, and it'll be more clear where this came from. TitleLevel is just the object a consumer can use to get predefined values for what they should pass into the headingLevel prop. I don't think you need any reference to TitleLevel at all in these components, you could use it maybe just in a unit test (e.g.<Accordion headingLevel={TitleLevel.h2}> ...)

But even for the unit test, since you'd have to import it from over in the Title folder, I don't think you necessarily need to use TitleLevel in this PR at all.

mturley · 2019-06-17T19:38:46Z

...-4/react-core/src/components/AboutModal/__snapshots__/AboutModalBoxCloseButton.test.tsx.snap

@@ -11,7 +11,6 @@ exports[`AboutModalBoxCloseButton Test 1`] = `
  >
    <TimesIcon
      color="currentColor"
-      noVerticalAlign={false}


I'm not sure why these unrelated snapshots are updating, @jschuler @redallen do you know what the deal is here?

react-icons was recently updated to include this prop, pull down latest from master and rerun yarn install

…by default. (patternfly#2270) Pie chart tooltips display the x value of each slice as a tooltip label by default. Because we are doing our own calculations on the data and manually setting the x-value, this can lead to tooltips displaying array index values. Also, fix a bug where ChartDonutThreshold accessors were being used for child ChartDonutUtilization data prop.

…y-react into iss1881_copy

jenny-s51 · 2019-06-18T15:26:37Z

final PR is #2290

jenny-s51 requested review from mturley, jgiardino and tlabaj June 14, 2019 14:46

jschuler reviewed Jun 14, 2019

View reviewed changes

jenny-s51 force-pushed the iss1881_copy branch from 80c0ee1 to ed463df Compare June 17, 2019 13:58

fix(accordion): adds prop to heading level of parent component

885b36e

jenny-s51 force-pushed the iss1881_copy branch from ed463df to 885b36e Compare June 17, 2019 17:08

This was referenced Jun 17, 2019

PF4 - use context api in page component #2208

Merged

fix(accordion): prop for heading level #2176

Closed

jenny-s51 and others added 2 commits June 17, 2019 14:28

fix(Accordion): TS; works as required

9db9ef3

feat(ContextSelector): Convert context selector to typescript (patter…

9a4f484

…nfly#2147) * Convert context selector to typescript * Fix linting and build error * Add demo and integration tests * Add typescript badge to docs * Made event argument optional

jenny-s51 requested a review from jschuler June 17, 2019 18:30

fix(Accordion): updated snapshots

bbdb763

mturley reviewed Jun 17, 2019

View reviewed changes

TheRealJon and others added 8 commits June 17, 2019 16:38

Fix test (patternfly#2263)

28a08ca

fix(accordion): refactor/update snapshots

c5aced0

fix(accordion): adds prop to heading level of parent component

e756f55

fix(Accordion): TS; works as required

3ca8aec

fix(Accordion): updated snapshots

27b006f

fix(accordion): refactor/update snapshots

c751683

Merge branch 'iss1881_copy' of https://github.com/jenny-s51/patternfl…

54e3ac8

…y-react into iss1881_copy

jenny-s51 mentioned this pull request Jun 18, 2019

fix(accordion): adds prop to heading level of parent component #2290

Merged

jenny-s51 closed this Jun 18, 2019

jenny-s51 deleted the iss1881_copy branch July 3, 2019 19:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(accordion): adds prop to heading level of parent component #2273

fix(accordion): adds prop to heading level of parent component #2273

jenny-s51 commented Jun 14, 2019 •

edited

Loading

jschuler Jun 14, 2019

mturley Jun 14, 2019

jschuler Jun 14, 2019

jschuler Jun 14, 2019

jenny-s51 Jun 14, 2019

mturley Jun 14, 2019

patternfly-build commented Jun 14, 2019

codecov-io commented Jun 17, 2019 •

edited

Loading

mturley left a comment

mturley Jun 17, 2019

mturley Jun 17, 2019

mturley Jun 17, 2019

jenny-s51 Jun 17, 2019 •

edited

Loading

mturley Jun 17, 2019

mturley Jun 17, 2019

mturley Jun 17, 2019

mturley Jun 17, 2019

jschuler Jun 17, 2019

jenny-s51 commented Jun 18, 2019 •

edited

Loading

fix(accordion): adds prop to heading level of parent component #2273

fix(accordion): adds prop to heading level of parent component #2273

Conversation

jenny-s51 commented Jun 14, 2019 • edited Loading

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

patternfly-build commented Jun 14, 2019

codecov-io commented Jun 17, 2019 • edited Loading

Codecov Report

mturley 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

jenny-s51 Jun 17, 2019 • edited Loading

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

jenny-s51 commented Jun 18, 2019 • edited Loading

jenny-s51 commented Jun 14, 2019 •

edited

Loading

codecov-io commented Jun 17, 2019 •

edited

Loading

jenny-s51 Jun 17, 2019 •

edited

Loading

jenny-s51 commented Jun 18, 2019 •

edited

Loading