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

Provide a changelog tab when this file is bundled in the package #12035

Merged
merged 9 commits into from
Sep 15, 2016
Merged

Provide a changelog tab when this file is bundled in the package #12035

merged 9 commits into from
Sep 15, 2016

Conversation

XVincentX
Copy link
Member

@XVincentX XVincentX commented Sep 14, 2016

The following PR is a clumsy way to implement #11940

Basically I've replicated the README logic as well trying to generalise it whenever possible.
I'd love to get your thoughts as well some hints about how to test this, eventually.

I put some notes in the code - I hope you might clarify those points.

//cc @joaomoreno

image

@msftclas
Copy link

Hi @XVincentX, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by Microsoft and real humans are currently evaluating your PR.

TTYL, MSBOT;

}
}

private openReadme(extension: IExtension) {
return this.loadContents(() => this.extensionReadme.get()
private openMarkdown(extension: IExtension, content: TPromise<string>, noContentCopy: string) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Given that readme and changelog are basically the same file, I've generalised the function so it can be used from both sides.

Copy link
Member Author

Choose a reason for hiding this comment

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

Is it ok for you to get a non resolved Promise as parameter?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah perfect. We don't need that extension argument any way, right? I must've left it behind somehow. You can remove that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably so. I'll fix that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in 23815a0

@coveralls
Copy link

Coverage Status

Coverage remained the same at 61.094% when pulling e38c873 on XVincentX:master into fc54f4b on Microsoft:master.

@joaomoreno joaomoreno added this to the September 2016 milestone Sep 14, 2016
@joaomoreno
Copy link
Member

Dude, that was FAST! I'll review it asap.

Copy link
Member

@joaomoreno joaomoreno left a comment

Choose a reason for hiding this comment

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

Looks great so far!

@@ -250,6 +254,7 @@ export class ExtensionEditor extends BaseEditor {
this.navbar.onChange(this.onNavbarChange.bind(this, extension), this, this.transientDisposables);
this.navbar.push(NavbarSection.Readme, localize('details', "Details"));
this.navbar.push(NavbarSection.Contributions, localize('contributions', "Contributions"));
this.navbar.push(NavbarSection.Changelog, localize('changelog', "Changelog"));
Copy link
Member

Choose a reason for hiding this comment

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

I'd like to show the Changelog section only if there is actually a Changelog to show.

You can add a hasChangelog(): boolean method to IExtension that will let you know this synchronously.

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes sense! Will fix!

Copy link
Member Author

Choose a reason for hiding this comment

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

@joaomoreno
I was wondering...giving we're fundamentally trying to get the extension changelog and this value is cached thanks to Cache<string>, I'd avoid to add a new method and try to get and store the value as I made here: e0f529fb8157366c32c590c0380d7b0bd693e3e7

Let me know your thoughts!

Copy link
Member

@joaomoreno joaomoreno Sep 15, 2016

Choose a reason for hiding this comment

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

The problem is that the network request could be slow and you'd show the link some time later. Not to mention that you should then hook up a cancellation request to that, lest the user clicks on another extension; you wouldn't want that request to still be running, right?

The hasChangelog() method may appear ugly, but it's definitely a good solution.

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes sense, I didn't think that scenario.
Thanks for the clarification, I'll fix that!

Copy link
Member Author

Choose a reason for hiding this comment

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

@joaomoreno
I'm a bit sad for that method, but that's the life!
Fixed in b98c3f7

Copy link
Member

Choose a reason for hiding this comment

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

It pays off in the long run. 👍 I'll give the changes a try now.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 61.129% when pulling 23815a0 on XVincentX:master into fc54f4b on Microsoft:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 61.129% when pulling e0f529fb8157366c32c590c0380d7b0bd693e3e7 on XVincentX:master into fc54f4b on Microsoft:master.

Copy link
Member

@joaomoreno joaomoreno left a comment

Choose a reason for hiding this comment

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

Awesome, thanks for your work!

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) to 61.117% when pulling b98c3f7 on XVincentX:master into 91115aa on Microsoft:master.

@XVincentX
Copy link
Member Author

@joaomoreno Thanks!

What's going to happen about remote file extension in the gallery? Can I do something about that?

@joaomoreno
Copy link
Member

Sure, I've updated the first comment in #11940 with the next steps.

@joaomoreno joaomoreno merged commit 505f2f3 into microsoft:master Sep 15, 2016
@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants