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

feat(alert): convert alert to TS #1978

Merged
merged 22 commits into from May 17, 2019
Merged

feat(alert): convert alert to TS #1978

merged 22 commits into from May 17, 2019

Conversation

@redallen
Copy link
Contributor

redallen commented May 13, 2019

What: Demo is already added. Closes #1978 #2023

Additional issues: Changing the declaration file allows the following import to typecheck correctly: @patternfly/patternfly/utilities/Accessibility/accessibility.css.

@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented May 13, 2019

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented May 13, 2019

Codecov Report

Merging #1978 into master will decrease coverage by 0.06%.
The diff coverage is 80%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1978      +/-   ##
==========================================
- Coverage   82.49%   82.42%   -0.07%     
==========================================
  Files         625      625              
  Lines        6888     6912      +24     
  Branches      108      122      +14     
==========================================
+ Hits         5682     5697      +15     
+ Misses       1156     1155       -1     
- Partials       50       60      +10
Flag Coverage Δ
#patternfly3 84.89% <ø> (ø) ⬆️
#patternfly4 78.92% <80%> (-0.14%) ⬇️
#patternflymisc 95.68% <ø> (ø) ⬆️
Impacted Files Coverage Δ
...re/src/components/Alert/AlertActionCloseButton.tsx 66.66% <66.66%> (ø)
...eact-core/src/components/Alert/AlertActionLink.tsx 71.42% <71.42%> (ø)
...ly-4/react-core/src/components/Alert/AlertIcon.tsx 90.9% <80%> (ø)
...ernfly-4/react-core/src/components/Alert/Alert.tsx 88.46% <88.46%> (ø)

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 3bc6415...f7205ae. Read the comment docs.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented May 13, 2019

Codecov Report

Merging #1978 into master will decrease coverage by 0.15%.
The diff coverage is 77.55%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1978      +/-   ##
==========================================
- Coverage    82.5%   82.34%   -0.16%     
==========================================
  Files         627      627              
  Lines        6926     6974      +48     
  Branches      108      133      +25     
==========================================
+ Hits         5714     5743      +29     
+ Misses       1162     1161       -1     
- Partials       50       70      +20
Flag Coverage Δ
#patternfly3 84.88% <ø> (ø) ⬆️
#patternfly4 78.76% <77.55%> (-0.3%) ⬇️
#patternflymisc 95.68% <ø> (ø) ⬆️
Impacted Files Coverage Δ
...ernfly-4/react-core/src/components/Title/Title.tsx 94.11% <ø> (ø) ⬆️
...re/src/components/Alert/AlertActionCloseButton.tsx 58.33% <58.33%> (ø)
...eact-core/src/components/Alert/AlertActionLink.tsx 71.42% <71.42%> (ø)
...ly-4/react-core/src/components/Alert/AlertIcon.tsx 90.9% <80%> (ø)
...ernfly-4/react-core/src/components/Alert/Alert.tsx 88% <88%> (ø)
...rnfly-4/react-core/src/components/Text/TextList.js
...atternfly-4/react-core/src/components/Text/Text.js
...y-4/react-core/src/components/Text/TextListItem.js
...ly-4/react-core/src/components/Text/TextContent.js
...-4/react-core/src/components/Text/TextListItem.tsx 84.61% <0%> (ø)
... and 6 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 ae8f20f...1802808. Read the comment docs.

Copy link
Member

seanforyou23 left a comment

Can we rename component files instead of deleting and creating new? This will be helpful for in the future when we need to take a look back through the history of a component.

@redallen

This comment has been minimized.

Copy link
Contributor Author

redallen commented May 13, 2019

@seanforyou23 I think that's a thing left up to Git, not me. In your Title PR it showed up the same way :/

@seanforyou23

This comment has been minimized.

Copy link
Member

seanforyou23 commented May 13, 2019

@redallen did you git mv when you converted those files? In the link you pointed to it looks like I didn't do this either and should have. I see a few of these files were correctly renamed though like the test and AlertIcon so just wondering what's different. Since we're converting a lot of components over the next short while, I think it would be good to keep the traces intact if possible.

Copy link
Contributor

dlabaj left a comment

Few comments.

@dlabaj

This comment has been minimized.

Copy link
Contributor

dlabaj commented May 13, 2019

This is also missing integration tests for Alert.

@dlabaj

This comment has been minimized.

Copy link
Contributor

dlabaj commented May 13, 2019

@redallen @seanforyou23 I thought a git move just deleted them... I'm not sure how git handles the history with a git mv

@redallen redallen force-pushed the redallen:ts/alert branch from ba3a342 to b19558c May 14, 2019
@seanforyou23

This comment has been minimized.

Copy link
Member

seanforyou23 commented May 14, 2019

@redallen @dlabaj git mv ./oldFileName.js ./newFileName.tsx works as expected. See this example PR #2008

Since we're going through and converting all the PF4 react components to TypeScript, I think it's critical we leave these traces in place for every component. Otherwise, we'll lose all the history and this would be a shame.

Once you've moved (and committed) the file with the new extension, you can verify the history is in tact with something like git log --follow -p -- ./Alert.tsx. If it doesn't show the history, we've broken the linkage.

---
import { Alert, AlertActionLink, AlertActionCloseButton } from '@patternfly/react-core';
import './alert.scss';
import './examples/alert.scss';

This comment has been minimized.

Copy link
@dgutride

dgutride May 14, 2019

Member

I would really like to see these stay in /examples

This comment has been minimized.

Copy link
@redallen

redallen May 14, 2019

Author Contributor

Yeah, we can fix that in a separate PR using explicit frontmatter.

@dlabaj

This comment has been minimized.

Copy link
Contributor

dlabaj commented May 14, 2019

@redallen Not sure why but seeing this when I do a local build and run start:pf4 but not seeing it in doc site you have attach to this PR. Any thoughts?

Screen Shot 2019-05-14 at 1 15 23 PM

@redallen

This comment has been minimized.

Copy link
Contributor Author

redallen commented May 14, 2019

@dlabaj That error has been there since I started working on Patternfly-react. variantLabel gets propagated via {...props} somewhere.

redallen added 4 commits May 14, 2019
Copy link
Contributor

tlabaj left a comment

Can you also add the relate issue to the PR please

@tlabaj tlabaj added the chore 🏠 label May 15, 2019
Copy link
Contributor

dlabaj left a comment

LGTM

Copy link
Contributor

tlabaj left a comment

@redallen I think you meant to link issue #1978

Copy link
Contributor

tlabaj left a comment

A couple more comments.

@@ -0,0 +1,23 @@
describe('Alert Demo Test', () => {

This comment has been minimized.

Copy link
@tlabaj

tlabaj May 16, 2019

Contributor

Can you please add some test to verify the action buttons as well.

This comment has been minimized.

Copy link
@dlabaj

dlabaj May 16, 2019

Contributor

I will add this test. Here's the issue for it #2026

@redallen redallen force-pushed the redallen:ts/alert branch from 4fa0a57 to 9556a29 May 16, 2019
redallen added 3 commits May 16, 2019
@@ -177,3 +178,48 @@ class DangerAlert extends React.Component {
}
}
```

## Alert with custom variantLabel

This comment has been minimized.

Copy link
@tlabaj

tlabaj May 16, 2019

Contributor

can you remove this example. We try to keep parity with core.

Copy link
Contributor

tlabaj left a comment

A couple more comments. Also the you are still linking to the About Modal conversion issue.

const readerTitle = (
<React.Fragment>
<span className={css(accessibleStyles.screenReader)}>
{variantLabel || `${capitalize(variant)} alert:`}

This comment has been minimized.

Copy link
@tlabaj

tlabaj May 16, 2019

Contributor

Why are you are concatenating the string here and not with he default props similar to aria-label?

This comment has been minimized.

Copy link
@redallen

redallen May 16, 2019

Author Contributor

It's not concatenation. It's the same as

variantLabel ? variantLabel : `${capitalize(variant)} alert:`

This comment has been minimized.

Copy link
@dlabaj

dlabaj May 16, 2019

Contributor

I think it can be updated on line 36 to be

Suggested change
{variantLabel || `${capitalize(variant)} alert:`}
variantLabel = `${capitalize(variant)} alert:`

and changed here to:

Suggested change
{variantLabel || `${capitalize(variant)} alert:`}
{variantLabel}

this way the consumers would know what we are defaulting too if they don't provide any variantLabel

redallen added 2 commits May 16, 2019
@dlabaj
dlabaj approved these changes May 16, 2019
Copy link
Contributor

dlabaj left a comment

Good job. Thanks @redallen !

Copy link
Contributor

tlabaj left a comment

LGTM

@redallen redallen dismissed stale reviews from tlabaj and dlabaj via 1802808 May 16, 2019
@dlabaj
dlabaj approved these changes May 16, 2019
@tlabaj
tlabaj approved these changes May 16, 2019
Copy link
Contributor

tlabaj left a comment

LGTM

@tlabaj tlabaj merged commit 72a86f3 into patternfly:master May 17, 2019
2 checks passed
2 checks passed
ci/circleci: build Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.