-
Notifications
You must be signed in to change notification settings - Fork 66
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
[BD-46] feat: defined types for Paragon exports #2177
[BD-46] feat: defined types for Paragon exports #2177
Conversation
Thanks for the pull request, @PKulkoRaccoonGang! When this pull request is ready, tag your edX technical lead. |
❌ Deploy Preview for paragon-openedx failed.Built without sensitive environment variables
|
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #2177 +/- ##
==========================================
- Coverage 91.65% 91.64% -0.01%
==========================================
Files 236 236
Lines 4195 4203 +8
Branches 1012 1013 +1
==========================================
+ Hits 3845 3852 +7
- Misses 346 347 +1
Partials 4 4
☔ View full report in Codecov by Sentry. |
bf4cc60
to
996bc0d
Compare
c774131
to
aae4443
Compare
48a0248
to
597bb37
Compare
@@ -2,17 +2,25 @@ import React from 'react'; | |||
import PropTypes from 'prop-types'; | |||
import classNames from 'classnames'; | |||
|
|||
export interface ActionRowProps { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[clarification] Can you expand on why the ActionRowProps
interface is defined in both this index.tsx
file and the index.d.ts
file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed .d.ts
for ActionRow
since it has .tsx
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@monteri [clarification] So, the introduction of the .d.ts
files are only necessary if the component is not already written in .tsx
? I imagine we would eventually intend to migrate our other components' .jsx
files to .tsx
files. If we do that, what would happen to the .d.ts
files? Would they remain, or get moved into the migrated .tsx
files?
I guess I'm just wondering about consistency around where we define types across components (i.e., always included in the .d.ts
, always included in the .tsx
, or similar).
dfdf4d0
to
8efc2a8
Compare
…, Popover components
e0f50d2
to
15d08f6
Compare
@PKulkoRaccoonGang Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future. |
Description
Issue: #1964
Merge Checklist
example
app?wittjeff
andadamstankiewicz
as reviewers on this PR.Post-merge Checklist