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

chore(nomenclature): phrase non production components as beta #3663

Merged
merged 1 commit into from Feb 10, 2020

Conversation

seanforyou23
Copy link
Collaborator

What: Steps toward closing #3624

This PR renames "experimental" things to "beta" so that the intent of the in-progress status is more clearly signaled to users.

@patternfly-build
Copy link
Contributor

patternfly-build commented Feb 5, 2020

Copy link
Contributor

@redallen redallen left a comment

Choose a reason for hiding this comment

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

Looks like some file renaming went awry?

 packages/patternfly-4/react-core/src/beta/components/DataToolbar/DataToolbar.test.tsx:13:53 - error TS2307: Cannot find module '../../../../components/Select'.

    13 import { Select, SelectOption, SelectVariant } from '../../../../components/Select';
                                                           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Copy link
Contributor

@redallen redallen left a comment

Choose a reason for hiding this comment

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

CI is sad

@codecov-io
Copy link

codecov-io commented Feb 5, 2020

Codecov Report

Merging #3663 into master will decrease coverage by 0.04%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3663      +/-   ##
==========================================
- Coverage   71.26%   71.21%   -0.05%     
==========================================
  Files         779      779              
  Lines       10489    10489              
  Branches     2261     2261              
==========================================
- Hits         7475     7470       -5     
  Misses       2592     2592              
- Partials      422      427       +5
Flag Coverage Δ
#misc 95.45% <ø> (ø) ⬆️
#patternfly3 85.89% <ø> (ø) ⬆️
#patternfly4 60.11% <ø> (-0.09%) ⬇️
Impacted Files Coverage Δ
...onents/DataToolbar/DataToolbarChipGroupContent.tsx 53.84% <ø> (ø)
...re/src/components/DataToolbar/DataToolbarGroup.tsx 100% <ø> (ø)
...-core/src/components/Drawer/DrawerPanelContent.tsx 80% <ø> (ø)
...e/src/components/DataToolbar/DataToolbarFilter.tsx 75% <ø> (ø)
...nents/DataToolbar/DataToolbarExpandableContent.tsx 88.88% <ø> (ø)
...ore/src/components/DataToolbar/DataToolbarItem.tsx 83.33% <ø> (ø)
.../components/DataToolbar/DataToolbarToggleGroup.tsx 47.05% <ø> (ø)
...nfly-4/react-core/src/components/Drawer/Drawer.tsx 71.42% <ø> (ø)
...ct-core/src/components/DataToolbar/DataToolbar.tsx 64.51% <ø> (ø)
.../src/components/DataToolbar/DataToolbarContent.tsx 100% <ø> (ø)
... and 4 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 9ac74fe...68b2bfd. Read the comment docs.

redallen
redallen previously approved these changes Feb 5, 2020
Copy link
Member

@dgutride dgutride left a comment

Choose a reason for hiding this comment

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

Two big things - should we just import directly from react-core instead of the beta path to reduce changes down the road (and now that we can) and can the beta category be removed from the gatsby doc output yet?

packages/patternfly-4/react-docs/gatsby-browser.js Outdated Show resolved Hide resolved
packages/patternfly-4/react-docs/gatsby-config.js Outdated Show resolved Hide resolved
packages/patternfly-4/react-styled-system/README.md Outdated Show resolved Hide resolved
packages/react-codemods/README.md Outdated Show resolved Hide resolved
Copy link
Member

@dgutride dgutride left a comment

Choose a reason for hiding this comment

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

The experimental/index.ts barrel file needs to be added back as well (and export from ../beta/)

@seanforyou23
Copy link
Collaborator Author

I've pushed some updates. We now display previously "experimental" components alongside all production components and visually mark them as "Beta" with an inline alert. This alert message needs to be reviewed.

Screen Shot 2020-02-07 at 12 35 17 PM

In addition to now having the notion of beta components, we also have support for beta properties/features on production components. No beta properties exist yet, but below is a screenshot of how they are currently wired up to display. Notice the property name is exampleBetaProperty and the visual treatment is to simply prefix the property name with Beta: , just like we do for deprecated properties. We could style it some special way, or instead just generate a new "beta" column, but I figured a first pass would be good to follow the existing convention. #3581, which is in-progress, will put this new feature to use for the first time, I'm happy to address any changes now or later to this part of the experience.

Screen Shot 2020-02-07 at 12 26 20 PM

Copy link
Member

@mcarrano mcarrano left a comment

Choose a reason for hiding this comment

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

@seanforyou23 just a couple things... The inline alert looks good. I checked out the Data toolbar page. However looks like there may be a problem pulling it the correct styles? The layout of this component is all messed up.

For tagging a property as Beta, is it possible to get closer to @rachael-phillips 's design using a badge?

@seanforyou23
Copy link
Collaborator Author

Thanks @mcarrano - checking on the styles, think we just need to pull in a recent change from core. Good catch.

As far as using a badge to flag beta properties, it shouldn't be a problem. I will take a look!

@seanforyou23 seanforyou23 force-pushed the experimental-beta branch 2 times, most recently from b4a52ff to 7fcc807 Compare February 10, 2020 17:43
@seanforyou23
Copy link
Collaborator Author

Thx for the feedback @mcarrano - I pulled in the latest changes from core and that seems to have fixed the styles on the Data toolbar/Drawer beta component pages. Also, made some changes in an open PR in org that should better reflect the design @rachael-phillips provided for beta styles (sry, didn't see that before). Here's an example of what those changes across org/core produce:

Marking examples as beta:
Screen Shot 2020-02-10 at 1 14 29 PM

And then marking props as beta in the props table at the bottom:
Screen Shot 2020-02-10 at 1 28 10 PM

Let me know if you see any issues!

@redallen redallen merged commit c34a6f4 into patternfly:master Feb 10, 2020
@patternfly-build
Copy link
Contributor

Your changes have been released in:

  • @patternfly/react-catalog-view-extension@1.3.13
  • @patternfly/react-charts@5.2.27
  • @patternfly/react-core@3.136.10
  • @patternfly/react-docs@4.18.15
  • @patternfly/react-inline-edit-extension@2.16.12
  • demo-app-ts@3.22.10
  • @patternfly/react-styled-system@3.7.32
  • @patternfly/react-styles@3.6.30
  • @patternfly/react-table@2.26.12
  • @patternfly/react-tokens@2.7.29
  • @patternfly/react-topology@2.13.12
  • @patternfly/react-virtualized-extension@1.3.108
  • @patternfly/react-icons@3.14.42

Thanks for your contribution! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants