Skip to content

Conversation

@lockee14
Copy link
Contributor

@lockee14 lockee14 commented Jun 23, 2019

Description

Feature: #3064
Add an option in preference > interface to toggle the iso 8601 format for the Date shortcut using ctrl-/ and ctrl-shift-/

Issue fixed

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/video to visualize my change if possible

dateISO8601


IssueHunt Summary

IssueHunt has been backed by the following sponsors. Become a sponsor

@ZeroX-DG ZeroX-DG added the awaiting review ❇️ Pull request is awaiting a review. label Jun 24, 2019
defaultNote: 'ALWAYS_ASK', // 'ALWAYS_ASK', 'SNIPPET_NOTE', 'MARKDOWN_NOTE'
showMenuBar: false
showMenuBar: false,
dateFormatISO8601: false
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn’t it be editor instead of ui?

: null
}

<div styleName='group-checkBoxSection'>
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 you should move this config down to editor setting

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as you asked me I put the config in the editor setting

@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 Jun 27, 2019
@ZeroX-DG ZeroX-DG changed the title Add Date shortcut ISO 8601 format as an option in preference, solve > https://github.com/BoostIO/Boostnote/issues/3064 Add Date shortcut ISO 8601 format as an option in preference Jul 1, 2019
: null
}

{/* <div styleName='group-checkBoxSection'>
Copy link
Member

Choose a reason for hiding this comment

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

Please remove the code instead of just comment it out. Thank you 👍

saveTagsAlphabetically: this.refs.saveTagsAlphabetically.checked,
enableLiveNoteCounts: this.refs.enableLiveNoteCounts.checked,
showMenuBar: this.refs.showMenuBar.checked,
// dateFormatISO8601: this.refs.dateFormatISO8601.checked,
Copy link
Member

Choose a reason for hiding this comment

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

Please remove the code instead of just comment it out. Thank you 👍

disableDirectWrite: false,
defaultNote: 'ALWAYS_ASK', // 'ALWAYS_ASK', 'SNIPPET_NOTE', 'MARKDOWN_NOTE'
showMenuBar: false
// dateFormatISO8601: false
Copy link
Member

Choose a reason for hiding this comment

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

Please remove the code instead of just comment it out. Thank you 👍

switchPreview={config.editor.switchPreview}
enableMarkdownLint={config.editor.enableMarkdownLint}
customMarkdownLintConfig={config.editor.customMarkdownLintConfig}
// dateISO8601={config.ui.dateFormatISO8601}
Copy link
Member

Choose a reason for hiding this comment

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

Please remove the code instead of just comment it out. Thank you 👍

switchPreview={config.editor.switchPreview}
enableMarkdownLint={config.editor.enableMarkdownLint}
customMarkdownLintConfig={config.editor.customMarkdownLintConfig}
// dateISO8601={config.ui.dateFormatISO8601}
Copy link
Member

Choose a reason for hiding this comment

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

Please remove the code instead of just comment it out. Thank you 👍

enableSmartPaste={config.editor.enableSmartPaste}
hotkey={config.hotkey}
autoDetect={autoDetect}
// dateISO8601={config.ui.dateFormatISO8601}
Copy link
Member

Choose a reason for hiding this comment

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

Please remove the code instead of just comment it out. Thank you 👍

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'm sorry I forgot to remove those before committing, it's done.

@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 Jul 29, 2019
@ZeroX-DG
Copy link
Member

@lockee14 can you fix the conflict before I approve your code please?

@Flexo013 Flexo013 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 Aug 29, 2019
@lockee14
Copy link
Contributor Author

done

@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 Aug 30, 2019
@Flexo013
Copy link
Contributor

@lockee14 It seems 1 check has failed. Could you look into this and fix it?

missing indentation and brace between line 229 - 234:
before:
'Shift-Cmd-/': function (cm) {
        if (global.process.platform !== 'darwin') { return }
      [translateHotkey(hotkey.insertDateTime)]: function (cm) {
        const dateNow = new Date()
        cm.replaceSelection(dateNow.toLocaleString())
},

after:
'Shift-Cmd-/': function (cm) {
        if (global.process.platform !== 'darwin') { return }
        [translateHotkey(hotkey.insertDateTime)]: function (cm) {
          const dateNow = new Date()
          cm.replaceSelection(dateNow.toLocaleString())
        }
},
@lockee14
Copy link
Contributor Author

lockee14 commented Sep 11, 2019

it seems there is still an error at line 231 in CodeEditor.js
image

I thought I had fixed it by adding indentation and a brace, but there is still something missing and I don't see what

so there is something wrong here:

'Shift-Cmd-/': function (cm) {
        if (global.process.platform !== 'darwin') { return }
        [translateHotkey(hotkey.insertDateTime)]: function (cm) {
          const dateNow = new Date()
          cm.replaceSelection(dateNow.toLocaleString())
        }
      },

it should maybe be:

[translateHotkey(hotkey.insertDateTime)]: function (cm) {
        if (global.process.platform !== 'darwin') { return } // maybe also remove this?
          const dateNow = new Date()
          cm.replaceSelection(dateNow.toLocaleString())
},

I'm not sure because I don't remember this '[translateHotkey(hotkey.insertDateTime)]' and it has been a few months since I have done the Date ISO 8601 shortcut feature

@ZeroX-DG
Copy link
Member

Maybe it's because of this rule? This is so strange, it haven't happen before.
https://eslint.org/docs/rules/object-shorthand

I'll investigate it when I'm at home

@lockee14
Copy link
Contributor Author

by looking at it a second time I think I get it, the code should be:

[translateHotkey(hotkey.insertDateTime)]: function (cm) {
          const dateNow = new Date()
          cm.replaceSelection(dateNow.toLocaleString())
},

translateHotkey seems to be here to handle compatibility issue between mac/linux/windows for the 'cmd' key thus:

'Shift-Cmd-/': function (cm) { // this is not needed anymore since translateHotkey do the work
        if (global.process.platform !== 'darwin') { return } // not needed either
        [translateHotkey(hotkey.insertDateTime)]: function (cm) { // this raise an error since it's a hash key that point to a function without being contained in a proper object variable >> var = {hashkey: something}
          const dateNow = new Date()
          cm.replaceSelection(dateNow.toLocaleString())
        }
      },

I think it should solve the issue but I'd like a second look on it since I'm not familiar with the codebase

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.

@lockee14 can you fix your code according to my review?

cm.replaceSelection(dateNow.toLocaleDateString())
}
},
'Cmd-/': function (cm) {
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 you should remove the code for Cmd-/ and Shift-Ctrl-/ and replace that with [translateHotkey(hotkey.insertDateTime)]. You code should be something like this:

[translateHotkey(hotkey.insertDateTime)]: function (cm) {
  const dateNow = new Date()
  if (self.props.dateFormatISO8601) {
       cm.replaceSelection(dateNow.toISOString())
  } else {
       cm.replaceSelection(dateNow.toLocaleString())
  }
}

@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 Nov 28, 2019
@ZeroX-DG
Copy link
Member

ZeroX-DG commented May 4, 2020

@lockee14 ping!

@ZeroX-DG
Copy link
Member

@lockee14 ping 2

@ZeroX-DG
Copy link
Member

ping3 @lockee14 if you not interested in this PR anymore I'll take over it after a week 👍

@lockee14
Copy link
Contributor Author

Sorry for not answering I'm too busy with other things and I don't remember what I did, it was a long time ago...

@ZeroX-DG
Copy link
Member

That's alright, I'll take it over and finish it for you:) Unless u are interested in finish it yourself

@ZeroX-DG
Copy link
Member

ZeroX-DG commented Jul 1, 2020

Take over here #3600

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

awaiting changes 🖊️ Pull request has been reviewed, but contributor needs to make changes.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants