fix(accordion): adds prop to heading level of parent component by jenny-s51 · Pull Request #2273 · patternfly/patternfly-react · GitHub

What: closes #1881

this should not be here

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

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

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-React preview: https://2273-pr-patternfly-react-patternfly.surge.sh

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

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.

We can fully remove this now.

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

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

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

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.

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

final PR is #2290

jenny-s51 requested review from jgiardino, mturley 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

               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';

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

               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';

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

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

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


		export const AccordionContext = React.createContext('h3');

		export enum HeadingLevel {

                 >
                   <TimesIcon
                     color="currentColor"
-                    noVerticalAlign={false}

fix(accordion): adds prop to heading level of parent component by jenny-s51 · Pull Request #2273 · patternfly/patternfly-react · GitHub

Uh oh!

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

fix(accordion): adds prop to heading level of parent component#2273jenny-s51 wants to merge 12 commits intopatternfly:masterpatternfly/patternfly-react:masterfrom jenny-s51:iss1881_copyjenny-s51/patternfly-react:iss1881_copyCopy head branch name to clipboard

Conversation

jenny-s51 commented Jun 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

patternfly-build commented Jun 14, 2019

Uh oh!

codecov-io commented Jun 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mturley left a comment

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!

jenny-s51 Jun 17, 2019 • 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!

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!

jenny-s51 commented Jun 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

fix(accordion): adds prop to heading level of parent component#2273
jenny-s51 wants to merge 12 commits into
patternfly:masterfrom
jenny-s51:iss1881_copy

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