Skip to content

Add fix for carousel asBaseComponent #1011

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

Merged
merged 3 commits into from
Nov 1, 2020
Merged

Conversation

Dor256
Copy link
Contributor

@Dor256 Dor256 commented Nov 1, 2020

Description

Enter description to help the reviewer understand what's the change about...
Fix typing for carousel component

Changelog

Add a quick message for our users about this change
Fix typing issue with carousel component

@Dor256 Dor256 added the typescript typescript related issue label Nov 1, 2020
@Dor256 Dor256 requested review from ethanshar and M-i-k-e-l November 1, 2020 07:46
Copy link
Collaborator

@M-i-k-e-l M-i-k-e-l left a comment

Choose a reason for hiding this comment

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

AWSOME! 10x!

Could you please remove changes from CarouselScreen except the carousel = React.createRef<typeof Carousel>();, and run npm run build:dev?

Also, I've tried to see if it works in the private lib and I have a problem where the typings are working but the the actual ref. I hope that it will work once this is merged, we'll see.

@Dor256
Copy link
Contributor Author

Dor256 commented Nov 1, 2020

AWSOME! 10x!

Could you please remove changes from CarouselScreen except the carousel = React.createRef<typeof Carousel>();, and run npm run build:dev?

Also, I've tried to see if it works in the private lib and I have a problem where the typings are working but the the actual ref. I hope that it will work once this is merged, we'll see.

Do you mean that the ref typing doesn't work or the ref in runtime doesn't work? Runtime shouldn't be affected by this change

@Dor256 Dor256 requested a review from M-i-k-e-l November 1, 2020 11:47
@M-i-k-e-l
Copy link
Collaborator

AWSOME! 10x!
Could you please remove changes from CarouselScreen except the carousel = React.createRef<typeof Carousel>();, and run npm run build:dev?
Also, I've tried to see if it works in the private lib and I have a problem where the typings are working but the the actual ref. I hope that it will work once this is merged, we'll see.

Do you mean that the ref typing doesn't work or the ref in runtime doesn't work? Runtime shouldn't be affected by this change

Runtime, I need to change the code to fix the TS errors and then the runtime does not work (although the code is the same as in the screen here, weird).

@Dor256
Copy link
Contributor Author

Dor256 commented Nov 1, 2020

AWSOME! 10x!
Could you please remove changes from CarouselScreen except the carousel = React.createRef<typeof Carousel>();, and run npm run build:dev?
Also, I've tried to see if it works in the private lib and I have a problem where the typings are working but the the actual ref. I hope that it will work once this is merged, we'll see.

Do you mean that the ref typing doesn't work or the ref in runtime doesn't work? Runtime shouldn't be affected by this change

Runtime, I need to change the code to fix the TS errors and then the runtime does not work (although the code is the same as in the screen here, weird).

Weird... The only change was to the typing, this shouldn't break any runtime code

@M-i-k-e-l
Copy link
Collaborator

AWSOME! 10x!

Could you please remove changes from CarouselScreen except the carousel = React.createRef<typeof Carousel>();, and run npm run build:dev?

Also, I've tried to see if it works in the private lib and I have a problem where the typings are working but the the actual ref. I hope that it will work once this is merged, we'll see.

Did you run npm run build:dev? I don't think you should get the changes in the other files.
You might need to clean-install the node_modules. Let me know if you want me to do it (I'm sure you have other tasks :) )

@Dor256
Copy link
Contributor Author

Dor256 commented Nov 1, 2020

AWSOME! 10x!
Could you please remove changes from CarouselScreen except the carousel = React.createRef<typeof Carousel>();, and run npm run build:dev?
Also, I've tried to see if it works in the private lib and I have a problem where the typings are working but the the actual ref. I hope that it will work once this is merged, we'll see.

Did you run npm run build:dev? I don't think you should get the changes in the other files.
You might need to clean-install the node_modules. Let me know if you want me to do it (I'm sure you have other tasks :) )

I did, but I got an error for TouchableOpacity (which I have another PR to fix). If you want you can just checkout the branch and try it yourself

@M-i-k-e-l
Copy link
Collaborator

AWSOME! 10x!
Could you please remove changes from CarouselScreen except the carousel = React.createRef<typeof Carousel>();, and run npm run build:dev?
Also, I've tried to see if it works in the private lib and I have a problem where the typings are working but the the actual ref. I hope that it will work once this is merged, we'll see.

Did you run npm run build:dev? I don't think you should get the changes in the other files.
You might need to clean-install the node_modules. Let me know if you want me to do it (I'm sure you have other tasks :) )

I did, but I got an error for TouchableOpacity (which I have another PR to fix). If you want you can just checkout the branch and try it yourself

Weird, I did and not did again and it removed the other files and I had no error with TouchableOpacity...

@M-i-k-e-l
Copy link
Collaborator

Only thing now is perhaps add an interface Statics, what do you think?

@Dor256
Copy link
Contributor Author

Dor256 commented Nov 1, 2020

Only thing now is perhaps add an interface Statics, what do you think?

Up to you, it would be cleaner to add an interface like that to all components that have static members

@M-i-k-e-l
Copy link
Collaborator

Only thing now is perhaps add an interface Statics, what do you think?

Up to you, it would be cleaner to add an interface like that to all components that have static members

It seems that the usage is more cumbersome

@M-i-k-e-l M-i-k-e-l merged commit 087d7e6 into master Nov 1, 2020
@M-i-k-e-l M-i-k-e-l deleted the dor-carousel-fix-suggestion branch January 25, 2021 09:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
typescript typescript related issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants