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

Add Wakatime integration #2923

Merged
merged 16 commits into from
Jul 20, 2020
Merged

Add Wakatime integration #2923

merged 16 commits into from
Jul 20, 2020

Conversation

LuisReinoso
Copy link
Contributor

Description

Add Wakatime support to track time spend writing in Boostnote.
plugins-preferences
Result on wakatime.
storageLocation

Issue fixed

#2810

Type of changes

  • ⚪ Bug fix (Change that fixed an issue)
  • ⚪ Breaking change (Change that can cause existing functionality to change)
  • ⚪ Improvement (Change that improves the code. Maybe performance or development improvement)
  • ❌ Feature (Change that adds new functionality)
  • ⚪ Documentation change (Change that modifies documentation. Maybe typo fixes)

Checklist:

  • ❌ My code follows the project code style
  • ⚪ I have written test for my code and it has been tested
  • ❌ All existing tests have been passed
  • ❌ I have attached a screenshot/vido to visualize my change if possible

@ZeroX-DG ZeroX-DG added the awaiting review ❇️ Pull request is awaiting a review. label Mar 13, 2019
@ZeroX-DG
Copy link
Member

ZeroX-DG commented May 4, 2019

Can you fix the conflict? And also check if the wakatime-cli is installed on the user system? Maybe display a small alert saying, the wakatime-cli is not found, please install it.

@ZeroX-DG ZeroX-DG added awaiting changes 🖊️ Pull request has been reviewed, but contributor needs to make changes. and removed awaiting review ❇️ Pull request is awaiting a review. labels May 4, 2019
@LuisReinoso
Copy link
Contributor Author

Sure, I will do this weekend.

@ZeroX-DG
Copy link
Member

@LuisReinoso ping:) Do you still interested in this PR?

@LuisReinoso
Copy link
Contributor Author

Oh oh oh.. I was forgot it.. Yes

@ZeroX-DG
Copy link
Member

Haha no worries, glad you can contribute this feature 👍

@ZeroX-DG
Copy link
Member

@LuisReinoso ping

@ZeroX-DG
Copy link
Member

ZeroX-DG commented May 7, 2020

@LuisReinoso ping!

@LuisReinoso
Copy link
Contributor Author

LuisReinoso commented May 8, 2020

Something strange is happen. Why is fetching your this repo instead of my work?
imagen

I have a blocker.

Lint on my side
imagen

Lint on CI
imagen

@ZeroX-DG
Copy link
Member

ZeroX-DG commented May 9, 2020

@LuisReinoso The CI fetch the original repo first the check out to the branch that contain your PR:
image

I tried to checkout to your PR and my lint also result same as on CI

image

Which is very strange, have you run lint on the right branch? or try updating lint by delete node_modules and run yarn before running lint again

@LuisReinoso
Copy link
Contributor Author

Thanks! I will tried again. Maybe some cache.

@LuisReinoso
Copy link
Contributor Author

LuisReinoso commented May 16, 2020

I'm back I removed node_modules and installed again via yarn and it works.. Thanks..!

@LuisReinoso
Copy link
Contributor Author

Please review my changes. ;)

Copy link
Member

@ZeroX-DG ZeroX-DG left a comment

Choose a reason for hiding this comment

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

Can you fix your code and also provide i18n for this please?

@@ -1,6 +1,5 @@
{
"trailingComma": "es5",
"tabWidth": 2,
Copy link
Member

Choose a reason for hiding this comment

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

any reason why you changed this setting?

Copy link
Contributor Author

@LuisReinoso LuisReinoso May 23, 2020

Choose a reason for hiding this comment

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

Do you remember that I had an issue on CI step.?

Now it has the same as master branch:
https://github.com/BoostIO/Boostnote/blob/master/.prettierrc

{
  "singleQuote": true,
  "semi": false,
  "jsxSingleQuote": true
}

Copy link
Member

Choose a reason for hiding this comment

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

ah okay

}

checkPluginsRequirements() {
this.checkWakatimePluginRequirement()
Copy link
Member

Choose a reason for hiding this comment

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

This method online call one method, why can't we directly call checkWakatimePluginRequirement?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking on this page is named plugins so on future new user could use this function to check if their developed plugins has all requirements. But I will remove it.

isWrite,
hasFileChanges,
isFileChange
) {
Copy link
Member

Choose a reason for hiding this comment

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

The last 3 variable is all booleans. I suggest we change to object type for more clarification. For example, if we change this function to:

function sendWakatimeHeartBeat(storagePath, noteKey, storageName, {isWrite, hasFileChanges, isFileChange}) {
  ...
}

We can keep the code inside the same but when we call this function, we will call it like this:

sendWakatimeHeartBeat(
  storagePath,
  noteKey,
  storageName,
  { isWrite: true, hasFileChanges: false, isFileChange: true}
)

false,
false,
false
)
Copy link
Member

Choose a reason for hiding this comment

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

This part seem to be a bit repeative. Surely we can improve the quality of this code by doing something similar to this:

if (this.props.onChange) {
    this.props.onChange(editor)
}

const isWrite = !!this.props.onChange;
const hasFileChanges = isWrite;

if (storage) {
  sendWakatimeHeartBeat(
    storage.path,
    noteKey,
    storage.name,
    isWrite,
    hasFileChanges,
    false
  )
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

DONE, It was very old code XD

@LuisReinoso
Copy link
Contributor Author

Please review my new changes.
Thanks!

@Flexo013 Flexo013 added awaiting review ❇️ Pull request is awaiting a review. and removed awaiting changes 🖊️ Pull request has been reviewed, but contributor needs to make changes. labels May 24, 2020
Copy link
Member

@ZeroX-DG ZeroX-DG left a comment

Choose a reason for hiding this comment

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

Can you change your code and also add support translation for other languages too?


checkWakatimePluginRequirement() {
const { wakatime } = this.state.config
if (wakatime.isActive && !commandExists('wakatime-cli')) {
Copy link
Member

Choose a reason for hiding this comment

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

This is wrong command name 😄 Should be like this:

Suggested change
if (wakatime.isActive && !commandExists('wakatime-cli')) {
if (wakatime.isActive && !commandExists('wakatime')) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

<div styleName='root'>
<div styleName='group'>
<div styleName='group-header'>{i18n.__('Plugins')}</div>
<div styleName='group-section'>
Copy link
Member

Choose a reason for hiding this comment

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

I think Wakatime isn't the only plugin that we support so it's better to create a section for Wakatime in this plugin setting page. Just a simple section with title Wakatime and bellow is a check box saying enable wakatime? and below it is the text box for wakatime key. If we plan to only have 1 plugin then this plugin page should change the name to Wakatime which I think will be worst cus harder to change when we want to support more plugins

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done 👍

@ZeroX-DG ZeroX-DG added awaiting changes 🖊️ Pull request has been reviewed, but contributor needs to make changes. and removed awaiting review ❇️ Pull request is awaiting a review. labels May 27, 2020
@ZeroX-DG
Copy link
Member

ZeroX-DG commented Jun 1, 2020

@LuisReinoso ping

NOTE:
Just a simple section with title Wakatime and bellow is a check box saying enable wakatime? and below it is the text box for wakatime key.
@LuisReinoso
Copy link
Contributor Author

Please review my new changes :D

Copy link
Member

@ZeroX-DG ZeroX-DG left a comment

Choose a reason for hiding this comment

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

VERY COOL! I'll approve this. Thanks for this PR 😄

@ZeroX-DG ZeroX-DG added approved 👍 Pull request has been approved by sufficient reviewers. and removed awaiting changes 🖊️ Pull request has been reviewed, but contributor needs to make changes. labels Jun 8, 2020
@Rokt33r Rokt33r merged commit 9556417 into BoostIO:master Jul 20, 2020
@Rokt33r Rokt33r added this to the v0.16.0 milestone Jul 20, 2020
@Flexo013 Flexo013 mentioned this pull request Jul 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved 👍 Pull request has been approved by sufficient reviewers.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants