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

Defining typescript interface for options of method to make screenshots #6630

Merged
merged 8 commits into from
Jul 26, 2019
Merged

Conversation

Dok11
Copy link
Contributor

@Dok11 Dok11 commented Jul 26, 2019

@sebavan
Copy link
Member

sebavan commented Jul 26, 2019

Could the interface be moved within the screenshotTools file ? This helps preventing import from tools in ES6 when not necessary.

Thx

@Dok11
Copy link
Contributor Author

Dok11 commented Jul 26, 2019

@sebavan check it please 6b36704

@sebavan
Copy link
Member

sebavan commented Jul 26, 2019

LGTM :-) Could you add an entry in dist/preview release/what's new.md ? and all the build will be green and ready to go in.

@Dok11
Copy link
Contributor Author

Dok11 commented Jul 26, 2019

add an entry in dist/preview release/what's new.md ?

Seems I cant understand.. it is written here what I must not commit dist/preview
https://doc.babylonjs.com/how_to/how_to_start#do-not-commit

@sebavan
Copy link
Member

sebavan commented Jul 26, 2019

This is utterly confusing I agree and I will fix the doc ASAP, the what s new file is the only one that needs to be updated. This is our base to create the changelog for the next release.

@@ -15,6 +15,7 @@ import { PromisePolyfill } from './promise';
import { TimingTools } from './timingTools';
import { InstantiationTools } from './instantiationTools';
import { GUID } from './guid';
import { IScreenshotSize } from './screenshotTools';
Copy link
Contributor

Choose a reason for hiding this comment

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

The only problem is that we do not want to import the file automatically for size reason (treeshacking). Do you mind moving the interface in its own file?

Copy link
Contributor

@deltakosh deltakosh left a comment

Choose a reason for hiding this comment

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

Need a tiny change

@Dok11
Copy link
Contributor Author

Dok11 commented Jul 26, 2019

Do you mind moving the interface in its own file?

I wanted itm but cant understand system for defining interfaces. In my projects I usual define interfaces in self directory called interfaces and use only one export construction per one file.
But here I dont see similar system or any other. So I make interfaces folder in the /Misc.

@deltakosh
Copy link
Contributor

iInspectable is already in the misc folder so it is ok to drop that file there as well
no need for a new folder

@Dok11
Copy link
Contributor Author

Dok11 commented Jul 26, 2019

iInspectable is already in the misc folder so it is ok to drop that file there as well
no need for a new folder

But in this folder already have many interfeces, these may be moved into interfaces folder. I dont did it because my PR not about it and not about refeactoring

@deltakosh
Copy link
Contributor

I agree but let's do it later then. (Also you have a doc issue)

@Dok11
Copy link
Contributor Author

Dok11 commented Jul 26, 2019

Also you have a doc issue

I thinked documentation generated automatic, it is not right? Or you about what's new.md file?

@Dok11
Copy link
Contributor Author

Dok11 commented Jul 26, 2019

I updated file what's news. Is now all ok?

@deltakosh
Copy link
Contributor

You can see the checks here: https://github.com/BabylonJS/Babylon.js/pull/6630/checks

The documentation problem: "[18:09:46] Missing text for Interface : IScreenshotSize (id: 24186) babylon.d.ts:31576"

@Dok11
Copy link
Contributor Author

Dok11 commented Jul 26, 2019

Aaaand did I pass this challenge?

@deltakosh
Copy link
Contributor

YOU DID :)
Congrats my friend!

@deltakosh deltakosh merged commit eb74e0b into BabylonJS:master Jul 26, 2019
@Dok11
Copy link
Contributor Author

Dok11 commented Jul 26, 2019

Yes! Thanks man!
One contributor more haha =)

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.

3 participants