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

Created import/export tab #3146

Open
wants to merge 20 commits into
base: master
Choose a base branch
from

Conversation

bertomoore
Copy link

@bertomoore bertomoore commented Jul 24, 2019

Description

Import option allows the user to select the boostnote.config file that contains their preferences.
Export option allows the user to select a directory that will hold their boostnote.config file.

import-export-screenshot

Issue fixed

Issue #3114

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 the 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

IssueHunt Summary

Referenced issues

This pull request has been submitted to:


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

@ZeroX-DG ZeroX-DG added needs extra review 🔎 Pull request requires review from an additional reviewer. awaiting review ❇️ Pull request is awaiting a review. and removed needs extra review 🔎 Pull request requires review from an additional reviewer. labels Jul 26, 2019
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 allow user to choose a different file name other than boostnote.config? After all, it's just importing and exporting, the config file is still in the system, only the content changed. Thank you for your contribution 👍

@@ -133,7 +142,8 @@ class Preferences extends React.Component {
{target: 'INFO', label: i18n.__('About')},
{target: 'CROWDFUNDING', label: i18n.__('Crowdfunding')},
{target: 'BLOG', label: i18n.__('Blog'), Blog: this.state.BlogAlert},
{target: 'SNIPPET', label: i18n.__('Snippets')}
{target: 'SNIPPET', label: i18n.__('Snippets')},
{target: 'IMPEXP', label: i18n.__('Import/Export'), ImpExp: this.state.ImpExpAlert}
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 Import/Export is not a very suitable name, because in this case we only export config and nothing else. Do you have any idea for another name?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe 'Backup Config', 'Backup Settings', or 'Backup Preferences'

@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 Aug 15, 2019
@bertomoore
Copy link
Author

I removed the name check for 'boostnote.config' when importing files since @ZeroX-DG does have a point in saying that any valid file is fine. I placed a check just to make sure a file is chosen or it will give an alert saying the path is empty.

Out of the recommendations made by @Flexo013 I went with 'Backup Config' for the tab name; my first choice was 'Backup Preferences' but it turned out to be a bit too long in practice. The actual page title is now 'Backup Configuration'.

The titles for the fields now say 'Select Backup File' and 'Create Backup File' respectively. I kept 'Import' and 'Export' for the buttons since they seemed appropriate.

I changed the source code naming to 'backup' except for the instances where 'import' and 'export' were still more appropriate like the file path variables.

Let me know if there are other changes I should make to this PR. Thank you!

@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 16, 2019
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.

Please update your code. (I accidentally approve your code sorry 😢 )

try {
const externalConfig = JSON.parse(fs.readFileSync(this.state.newImport.path, 'utf-8'))
const { config } = this.state
config.hotkey = externalConfig.hotkey
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a inner try catch here to validate the config too? Because otherwise the error message will be File is not valid JSON. which will confuse the user.

`${this.state.newExport.path}/boostnote.config`,
JSON.stringify(this.state.config), (err) => {
if (err) {
return console.log(err)
Copy link
Member

Choose a reason for hiding this comment

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

How about adding some kind of way to notice the user when saving is successful or failed? Because otherwise a think called Gulf of evaluation will happens.

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

@bertomoore ping

@ZeroX-DG
Copy link
Member

Close due to inactivity

@ZeroX-DG ZeroX-DG closed this Feb 24, 2020
@bertomoore
Copy link
Author

I'm very sorry for the lack of response! I can have the changes up by March 9th if you would like to reopen the PR.

@Flexo013 Flexo013 reopened this Mar 1, 2020
@ZeroX-DG
Copy link
Member

@bertomoore are you still interested in this PR?

@bertomoore
Copy link
Author

@ZeroX-DG yes! Sorry I've been very distracted, but I'll work on this PR immediately

@bertomoore
Copy link
Author

I have updated the error message in the import to check if the error is related to a syntax error, which is the exception thrown by JSON.parse. If this may still conflict with validating the config, I'm happy to change the method of error checking. I also added an alert box whenever "boostnote.config" file is exported successfully. I appreciate your patience and look forward to hearing from you.

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 a bit please @bertomoore

<input styleName='backup-body-section-path-input'
ref='newImportPath'
placeholder={i18n.__('Select File')}
value={this.state.newImport.path}
Copy link
Member

Choose a reason for hiding this comment

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

Can you add readOnly to the input to remove the React warning please?

<div styleName='backup-body-section-path'>
<input styleName='backup-body-section-path-input'
ref='newExportPath'
placeholder={i18n.__('Select Folder')}
Copy link
Member

Choose a reason for hiding this comment

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

Can you add readOnly to the input to remove the React warning please?

border-color get-theme-var(theme, 'borderColor')

.backup-button-control
background-color get-theme-var(theme, 'button-backgroundColor')
Copy link
Member

Choose a reason for hiding this comment

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

For some reason, the variable button-backgroundColor doesn't exist in dark theme. Can you fix this too? thank you 👍

@bertomoore
Copy link
Author

Hello, I added the 'readonly' attributes to the input fields and fixed the issue with the dark button background not present. Let me know if there's anything else I can do. :)

@ZeroX-DG
Copy link
Member

@bertomoore seem like the eslint is failling? Can you check if this is on your side or is the Travis broken again?

@bertomoore
Copy link
Author

Sorry about that, I fixed the errors found on the lint now!

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 with my suggestions? And can you add translation for those label?

browser/main/modals/PreferencesModal/BackupTab.js Outdated Show resolved Hide resolved
browser/main/modals/PreferencesModal/BackupTab.js Outdated Show resolved Hide resolved
bertomoore and others added 3 commits May 6, 2020 23:11
Co-authored-by: Nguyen Viet Hung <viethungax@gmail.com>
Co-authored-by: Nguyen Viet Hung <viethungax@gmail.com>
Co-authored-by: Nguyen Viet Hung <viethungax@gmail.com>
@ZeroX-DG
Copy link
Member

@bertomoore can you add translations for those labels?/ :+1:

@bertomoore
Copy link
Author

Yes! I will have it up within the next 24 hours 👍🏽

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.

Just some tiny requests before I can approve this PR 😄

this.setState({
BackupAlert: {
type: 'success',
message: i18n.__('Successfully applied!')
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 miss the translation for this one


exportConfig(e) {
if (this.state.newExport.path === '') {
alert(i18n.__('Please choose a directory for export file.'))
Copy link
Member

Choose a reason for hiding this comment

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

Can you add translation for this one too?

i18n.__(
err instanceof SyntaxError
? 'File is not valid JSON.'
: 'Something went wrong. Please try again'
Copy link
Member

Choose a reason for hiding this comment

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

Can you add translation for this too?

JSON.stringify(this.state.config),
err => {
if (err) {
alert(i18n.__('Export failed. Please try again'))
Copy link
Member

Choose a reason for hiding this comment

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

Can you add translation for this?

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

Choose a reason for hiding this comment

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

Can you add translation for this one too?

@bertomoore
Copy link
Author

Sorry about overlooking those! I'll fix it now. :)

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.

LGTM 🎉

@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 May 19, 2020
@daiyam daiyam mentioned this pull request Jun 12, 2020
@Rokt33r Rokt33r added this to the v0.16.0 milestone Jul 20, 2020
@Rokt33r
Copy link
Member

Rokt33r commented Jul 20, 2020

@bertomoore I tried it but I think we need to improve its UI more. Rather than having file inputs, we can just put buttons and use electron's file save/open dialog. https://www.electronjs.org/docs/api/dialog

@Rokt33r Rokt33r added awaiting changes 🖊️ Pull request has been reviewed, but contributor needs to make changes. and removed approved 👍 Pull request has been approved by sufficient reviewers. labels Jul 20, 2020
@Rokt33r Rokt33r self-requested a review July 20, 2020 11:06
@Rokt33r Rokt33r modified the milestones: v0.16.0, v0.17.0 Jul 20, 2020
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.

None yet

4 participants