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

Add type definitions #94

Closed
wants to merge 15 commits into from

Conversation

FDiskas
Copy link

@FDiskas FDiskas commented Nov 10, 2017

Closes #91

@FDiskas FDiskas mentioned this pull request Nov 10, 2017
@Sharlaan
Copy link
Owner

Can you update or remake this PR or rebase, this time based on new master branch please ?

…rselectfield into feature/typedefinitions

# Conflicts:
#	package-lock.json
#	package.json
…rselectfield into feature/typedefinitions

# Conflicts:
#	package-lock.json
#	package.json
import {Component} from 'react';

export interface SelectFieldProps {
anchorOrigin?: any;
Copy link
Author

Choose a reason for hiding this comment

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

Why everywhere types are any?
KnisterPeter/react-to-typescript-definitions#439

Copy link
Author

Choose a reason for hiding this comment

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

and this file should be in ignore to :)

Copy link
Owner

Choose a reason for hiding this comment

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

Speaking to yourself ? ^^

Copy link
Author

Choose a reason for hiding this comment

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

Just info :)

@codecov-io
Copy link

codecov-io commented Nov 13, 2017

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #94      +/-   ##
==========================================
- Coverage   42.07%   40.97%   -1.11%     
==========================================
  Files           6        6              
  Lines         202      205       +3     
==========================================
- Hits           85       84       -1     
- Misses        117      121       +4
Impacted Files Coverage Δ
src/defaultProps.js 53.84% <0%> (-4.49%) ⬇️
src/utils.js 36% <0%> (-3.14%) ⬇️
src/SuperSelectField.js 38.84% <0%> (-0.33%) ⬇️
src/FloatingLabel.js 22.22% <0%> (ø) ⬆️
src/SelectionsPresenter.js 100% <0%> (ø) ⬆️
src/types/index.js 33.33% <0%> (+2.56%) ⬆️

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 4a72665...04b90b7. Read the comment docs.

@FDiskas
Copy link
Author

FDiskas commented Nov 13, 2017

Useless codecov comment. Anybody knows how to turn off commenting on pull requests?

@Sharlaan
Copy link
Owner

One question:
Have you tried installing and using both the es5 and module versions of material-ui-superselectifled, on a dummy page featuring react/react-dom/material-ui ?

I'm asking this following your modification of package.json properties "main" and "module".

@FDiskas
Copy link
Author

FDiskas commented Nov 13, 2017

I tried to npm run build and that index.js was missing 🐩
Logic is simple.

  • User installs material-ui-superselectfield
  • Users imports import * as Select from 'material-ui-superselectfield'
  • Webpack (or whatever is) will look in node_modules the directory material-ui-superselectfield
  • Opens modules package.json file and looks for "main" section, because user was not specified the file in import statment

@Sharlaan
Copy link
Owner

If you read NWB documentation as suggested few days ago, you would have learn that :
lib/ contains the es5 version, transpiled
es/ contains the module-compatible version, by extension untranspiled since module import is last EcmaScript feature implemented into nowadays browsers
so this means :

  1. you MUST include these folders in npm publish
  2. index.js allows the browser JS engine to get all needed files (reminder they are splitted !) by default
    So if you rename this file browser won't import properly

There is a third build, it were the original form of my project before this transfer to NWB: the UMD.
In this particular build case, yea all component files are concatenated into one.
We didnot include this one, es5 and module versions should be enough for most projects.

@FDiskas
Copy link
Author

FDiskas commented Nov 13, 2017

You should check the results after pre released
https://unpkg.com/material-ui-superselectfield/ and check the "main" section in package.json

@Sharlaan
Copy link
Owner

What your link is pointing at, is a "UMD" build.
As explained above, we decided to ditch this in favor of es5 and es6+ module versions...

@FDiskas
Copy link
Author

FDiskas commented Nov 13, 2017

Yes link is pointing to the latest npm module version. Try publishing the new version of current refactored environment. You will see some changes. And the "main" section of package.json will be wrong.
And yes - got the point what you talking about. But keep in mind that by using UMD is a good way to test for users your component for example in jsbin.com

…rselectfield into feature/typedefinitions

# Conflicts:
#	package.json
@FDiskas
Copy link
Author

FDiskas commented Nov 15, 2017

@Sharlaan any news about this?

Copy link
Owner

@Sharlaan Sharlaan left a comment

Choose a reason for hiding this comment

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

This PR is about type definition, not storybook. You shouldnot mix.

Storybook (and htis addon) are already scheduled, so useless to add this here or any PR.

package.json Outdated
@@ -70,6 +70,7 @@
"react-dom": "^16.1.0",
"react-router": "^4.2.0",
"react-router-dom": "^4.2.2",
"react-to-typescript-definitions": "^0.22.0"
"react-to-typescript-definitions": "^0.22.0",
"storybook-addon-material-ui": "^0.8.2"
Copy link
Owner

Choose a reason for hiding this comment

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

This PR is about type definition, not storybook. You shouldnot mix.

Storybook (and htis addon) are already scheduled, so useless to add this here or any PR.

Copy link
Author

Choose a reason for hiding this comment

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

I just merged the master branch

Copy link
Author

Choose a reason for hiding this comment

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

Copy link
Owner

Choose a reason for hiding this comment

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

Ah ok, probably Max started on storybook.

…rselectfield into feature/typedefinitions

# Conflicts:
#	package.json
…rselectfield into feature/typedefinitions

# Conflicts:
#	package-lock.json
…rselectfield into feature/typedefinitions

# Conflicts:
#	package-lock.json
@FDiskas
Copy link
Author

FDiskas commented Nov 20, 2017

DO something. Can't wait and fix merge conflicts to often

@KnisterPeter
Copy link

Added support for prop-types: KnisterPeter/react-to-typescript-definitions#439 (comment)

@FDiskas
Copy link
Author

FDiskas commented Nov 22, 2017

cool 👖

@FDiskas
Copy link
Author

FDiskas commented Dec 8, 2017

Conflicting files
package-lock.json

so why I'm not using npm any more. Use yarn instead :)

…ature/typedefinitions

# Conflicts:
#	package-lock.json
@FDiskas
Copy link
Author

FDiskas commented Dec 20, 2017

Note definitions should not be committed. They must be generated on each build.

I'm right?

@FDiskas FDiskas closed this Feb 27, 2018
@FDiskas FDiskas deleted the feature/typedefinitions branch February 27, 2018 12:40
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.

4 participants