Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix sub-component references in react-console #894

Merged

Conversation

mareklibra
Copy link
Contributor

@mareklibra mareklibra commented Nov 7, 2018

Follow-up for 1f3411a , resp #853.

Sub-components (like Grid.Col) are no more exported individually.

import { MenuItem } from 'patternfly-react';

// FIXME: DropdownButton is inaccessible after 1f3411a6e708e833e48921380ae736cf5f79862b commit
import DropdownButton from 'patternfly-react/dist/esm/components/Button/DropdownButton';
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jeff-phillips-18 , any idea?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mareklibra I will look into this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah... so this was broken for the patternfly-react packages by #853 like you say.
Unfortunately, I can think of only 2 fixes:

  1. Create directories for all subcomponents and move them there. This would, however, be a breaking change externally as anyone doing path imports would no longer find the subcomponent in the same location.

  2. Add subcomponents to the main component and reference them through that (only required in patternfly-react packages not externally). That is, Button would have Button.Dropdown and you would import Button and use that . Similar to Row and Col in Grid.

Can you think of any other options?

Copy link
Contributor Author

@mareklibra mareklibra Nov 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Create directories for all subcomponents and move them there.

I would avoid that as well.

That is, Button would have Button.Dropdown and you would import Button and use that . Similar to Row and Col in Grid.

This is probably the easiest fix if the encapsulation by #853 is really needed (I don't see much benefit of that).

It makes sense in case of Grid as Col and Row are real subcomponents.
But Button and Dropdown are equal in the hierarchy (just different specialisation).
So doing it properly, we should introduce Buttons and reference via that, including the recent Button.

Are there any other occurrences except buttons? If not or not many, I would not care and add Button.Dropdown. Otherwise, a systematical approach should be preferred.

Can you think of any other options?

Just more complicated ones (none preferred by me):

  • introduce reference-only subcomponents handling the paths. We would get the liberty to hand-pick subcomponents for export.
  • modify build to export/generate components based on re-exporting from the index.js files

@coveralls
Copy link

coveralls commented Nov 7, 2018

Pull Request Test Coverage Report for Build 3257

  • 23 of 23 (100.0%) changed or added relevant lines in 8 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.03%) to 81.402%

Totals Coverage Status
Change from base Build 3247: 0.03%
Covered Lines: 3864
Relevant Lines: 4475

💛 - Coveralls

@mareklibra mareklibra force-pushed the fixSubcomponentReferences branch 2 times, most recently from 79eab76 to 2425fc6 Compare November 9, 2018 08:52
@mareklibra
Copy link
Contributor Author

mareklibra commented Nov 9, 2018

@jeff-phillips-18 , @priley86 , any idea how to import components like DropdownButton after #853, please?

Former

import { DropdownButton } from 'patternfly-react';

is newly babelized to

import DropdownButton from 'patternfly-react/dist/esm/components/DropdownButton/DropdownButton';

which is missing.

@mareklibra
Copy link
Contributor Author

This will be rebased once #916 lands.

@mareklibra
Copy link
Contributor Author

Rebased and tested with #916.
Still blocked by #916 merge.

@mareklibra mareklibra changed the title fix: Fix sub-component references in react-console Fix sub-component references in react-console Nov 16, 2018
Follow-up for 1f3411a.

Sub-components (like Grid.Col) are no more exported individually.
@mareklibra
Copy link
Contributor Author

Rebased after #916 merge.
@jeff-phillips-18 , @dtaylor113 , can you please have a look?

@patternfly-build
Copy link
Contributor

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

@jeff-phillips-18 jeff-phillips-18 merged commit a1e4d4f into patternfly:master Nov 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants