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

Confirm deletion of blog #602

Merged
merged 1 commit into from
May 29, 2019
Merged

Confirm deletion of blog #602

merged 1 commit into from
May 29, 2019

Conversation

rfwatson
Copy link
Contributor

@rfwatson rfwatson commented May 28, 2019

Closes #601.

Ideally I'd like a less generic confirmation message - Are you sure you want to permanently delete this blog?. But assuming this won't be possible without new translations, I feel that it's probably not worth it. Thoughts welcome.

@rfwatson
Copy link
Contributor Author

Not sure why the CI is failing

elegaanz
elegaanz previously approved these changes May 28, 2019
Copy link
Member

@elegaanz elegaanz left a comment

Choose a reason for hiding this comment

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

Hello and thanks for your PR! Don't hesitate to change the message if you feel to do so, having one more string to translate is not a problem at all.

And the reason the CI fails is that it takes a lot of memory to compile Plume, and sometimes Circle CI containers just don't give us enough… so Rust is killed by the OOM-killer (so nothing to do with your code, it may work next time).

Otherwise, it works just fine. Thank you again!

@rfwatson
Copy link
Contributor Author

Thanks!

Looking at the previous commits, would you recommend going on Crowdin directly to generate new translations?

I’ll play with this further tomorrow.

@elegaanz
Copy link
Member

You don't need to go on Crowdin to add translations: any string written in the i18n! macro is uploaded there automatically, and can then be translated (once your PR is merged, of course).

@codecov
Copy link

codecov bot commented May 29, 2019

Codecov Report

Merging #602 into master will increase coverage by 0.06%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           master    #602      +/-   ##
=========================================
+ Coverage   34.54%   34.6%   +0.06%     
=========================================
  Files          67      67              
  Lines        7825    7840      +15     
  Branches     1881    1883       +2     
=========================================
+ Hits         2703    2713      +10     
- Misses       4361    4367       +6     
+ Partials      761     760       -1

@rfwatson
Copy link
Contributor Author

You don't need to go on Crowdin to add translations: any string written in the i18n! macro is uploaded there automatically

Oh nice 😃

In that case I've updated this PR, and opened another to clarify the docs.

@elegaanz
Copy link
Member

Thank you again!

@elegaanz elegaanz merged commit bffce04 into Plume-org:master May 29, 2019
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.

Confirmation before deleting a blog
2 participants