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

Sending draft status in output from PL Editor #2677 #138

Merged
merged 8 commits into from
May 17, 2018
Merged

Sending draft status in output from PL Editor #2677 #138

merged 8 commits into from
May 17, 2018

Conversation

poppashingles
Copy link
Contributor

I had to delete the package-lock file and rebuild it, so it's updated a lot of the dependencies. All 25 tests are still passing ok on my local version.


output.authenticity_token = data.token || null;
// Whether note will be draft or not
output.draft = data.draft || false;
Copy link
Member

Choose a reason for hiding this comment

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

Awesome !!! how about introducing new lines aroud these 2 lines as it's looking bit of messy. Thanks @poppashingles .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

New lines added, thanks!

@@ -1,6 +1,6 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

Can't say about this. Let's ask @jywarren about it.

Copy link
Member

Choose a reason for hiding this comment

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

Its still there, could you please try one more time to remove it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hiya. In my forked one the package lock is gone and the pull request seems to be saying everything in it is gone. Am I reading it wrong?

@jywarren
Copy link
Member

jywarren commented May 8, 2018

Yes, package-lock.json is not needed -- do you think you could remove it? Also, if it's possible to make these changes to the files in /src/ rather than /dist/ -- basically dist files are auto-generated from src files, so this change would be come overwritten, does that make sense?

But looking great, thank you both!

@poppashingles
Copy link
Contributor Author

Thanks @jywarren :) I've removed the package-lock and moved the code into the PublicLab.Formatter.js file in src/adapters. Thanks again!

output.authenticity_token = data.token || null;

// Whether note will be draft or not
output.draft = data.draft || false;
Copy link
Member

Choose a reason for hiding this comment

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

Awesome @poppashingles !!!

@grvsachdeva
Copy link
Member

grvsachdeva commented May 15, 2018

@poppashingles how about opening a new PR just by adding lines in the concerned file from Github so that we don't get any extra additions and deletions? What do you think?

@grvsachdeva
Copy link
Member

hi @poppashingles , I have resolved the conflict with the package-lock.json file. Thanks for your help. Also, you have made changes in the master branch of your PR, that's a wrong way. The right way(from my knowledge) is to make a new branch, then make changes and commit to that branch and final step is raise a PR.

@grvsachdeva
Copy link
Member

hey @jywarren its ready now. Please review it. thanks.

@grvsachdeva grvsachdeva requested a review from jywarren May 15, 2018 14:19
@poppashingles
Copy link
Contributor Author

Thanks guys. Sorry it's not gone particularly smoothly! Been a really good learning experience though so thank you :)

@grvsachdeva
Copy link
Member

grvsachdeva commented May 16, 2018

No issue @poppashingles, we all are here for learning and I will also try to open more clear FTO from next time, this time I messed up a little. Thanks for the help! Also, if you like to solve more issues, you can find them here https://publiclab.github.io/community-toolbox/#r=all

@jywarren
Copy link
Member

Hi, all - thanks so much @poppashingles and @Gauravano for taking this one on! I think it just needs one more run of grunt build to get the final changes from /src/ to be compiled into /dist/ properly. I don't see them in the changes (they should match the changes from /src/, so if one of you could give that a try, I think that should be our last step!

Again, great work and thank you!

@grvsachdeva
Copy link
Member

hi @jywarren , I have run the grunt build. please review it again. Thanks.

@jywarren
Copy link
Member

Awesome, merging now then!

@jywarren jywarren merged commit 5495b24 into publiclab:master May 17, 2018
@jywarren
Copy link
Member

For this to show up on publiclab.org we now need to bump the version number by a 0.x.0 increment (see semver.org) and create a new release.

@jywarren
Copy link
Member

jywarren commented May 17, 2018 via email

@grvsachdeva
Copy link
Member

Hey, @jywarren thanks for merging this. I have checked locally, changes are in place.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants