Issue 1706: Option to rename tags #2989

khaliqgant · 2019-05-03T15:43:21Z

Description

Tags at present moment cannot be renamed. This PR reuses the "RenameFolderModal" to allow a user to rename a tag that is selected.

This update also renames the "RenameFolderModal.styl" to "RenameModal.styl" since the RenameTagModal.js uses that style as well and I felt it was clearer to use a more generic name. This is my first PR to the project (be kind! 😬) so I wasn't sure if that is something the maintainers would agree with or not.

Issue fixed

Fixes #1706

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

• repurpose RenameFolderModal.styl to RenameModal so it is more generic

ZeroX-DG · 2019-05-04T14:17:39Z

browser/main/modals/RenameTagModal.js

+    }
+  }
+
+  handleConfirmButtonClick (e) {


Why don't you call this.confirm directly instead of making a whole new method for it?

Thanks for looking it over! Updated.

…onfirm method

AWolf81 · 2019-05-08T17:34:49Z

browser/main/modals/RenameTagModal.js

+        <div styleName='header'>
+          <div styleName='title'>{i18n.__('Rename Tag')}</div>
+        </div>
+        <ModalEscButton handleEscButtonClick={(e) => this.handleCloseButtonClick(e)} />


Nit. I think handleCloseButtonClick method is not needed here.
You could destructure prop close with const { close } = this.props and do

Suggested change

<ModalEscButton handleEscButtonClick={(e) => this.handleCloseButtonClick(e)} />

<ModalEscButton handleEscButtonClick={close} />

Fixed in e985f12

AWolf81 · 2019-05-08T17:43:02Z

browser/main/modals/RenameTagModal.js

+        <div styleName='control'>
+          <input styleName='control-input'
+            placeholder={i18n.__('Tag Name')}
+            ref='name'


I think it would be better to use callback refs as it's likely that this will be deprecated in a future React version. Please use ref={ref => this.nameInput=ref} & add this.nameInput = null to the constructor & update the references from this.refs.name to the new property.

Docs about string refs leagcy api

Fixed in b1d62c8

AWolf81 · 2019-05-08T17:52:47Z

browser/main/modals/RenameTagModal.js

+            placeholder={i18n.__('Tag Name')}
+            ref='name'
+            value={this.state.name}
+            onChange={(e) => this.handleChange(e)}


Nit. Please use .bind(this) for onChange, onKeyDown and onClick so you can write e.g. onChange={this.handleChange}.
But this requires this.handleChange = this.handleChange.bind(this) in the constructor - we could probably also use class properties but I think that's not used in the code base.
I'm also not sure if it's supported by the Babel setup - I haven't checked.

Fixed in ce5d4ac

AWolf81 · 2019-05-08T18:50:18Z

Looks great. Just some smaller inline comments & one point would be awesome if you could add.

If you're selecting a tag and then rename it - the location is wrong and no notes are displayed.

By adding the following code to your renameTag method before closing the modal you can redirect to the selected tag to correctly update:

import { hashHistory } from 'react-router' // add to imports

renameTag (tag, updatedTag) {
  Promise
    .all(notes.map(note => dataApi.updateNote(note.storage, note.key, note)))
    .then(() => { /* ... */ })
    .then(() => {
      if (window.location.hash.includes(tag)) {
        hashHistory.replace(`/tags/${updatedTag}`)
      }
      this.props.close()
    })
}

…06-tag-rename

… assignment

…er via the TagSelect component

khaliqgant · 2019-05-11T15:10:28Z

Thanks for the detailed feedback @AWolf81!

Great tip with your comment above as it exposed an issue with the topbar tag

not updating when the tag was renamed. I added in an event (not sure if is the optimal way, but the best I could come up with at the time) to keep that portion up to date. Result below:

…06-tag-rename

khaliqgant · 2019-05-24T15:46:29Z

Branch has been brought up to date with the latest master

khaliqgant · 2019-06-13T07:19:59Z

Just a friendly ping on this @ZeroX-DG 😀

ZeroX-DG · 2019-07-27T00:05:22Z

Hi @khaliqgant , sorry for the late response.

Can you change the color of the modal so that it will match the theme? For example, here is how it look on monokai theme

Also a note can't have duplicated tags but if you do like this, the bug will appear:

Add a tag called tag1 to a note
Add a tag called tag2 to that same note
Rename tag2 to tag1
The note will now have duplicated tags.

…06-tag-rename

khaliqgant · 2019-09-15T08:29:37Z

Thanks for the feedback @ZeroX-DG.

Can you change the color of the modal so that it will match the theme?

Updated with 9a2dcbc

Also a note can't have duplicated tags but if you do like this, the bug will appear:

Updated with 4efbe08 and added in an error message to assist the user as seen in the video:

ZeroX-DG · 2019-09-17T00:58:42Z

@khaliqgant This is not quite what I wanted, I think it's better to merge tag into the existing tag if there's any. If the user want to rename tag computer to computers which already existed, the app should display a message showing the tag computers already existed, renaming this will make note with tag computer become computers and still allow user to rename it if it's what the user want.

ZeroX-DG · 2020-01-30T02:11:20Z

@khaliqgant ping

ZeroX-DG · 2020-02-12T01:07:13Z

@khaliqgant ping 2

ZeroX-DG · 2020-03-22T00:47:55Z

Close due to inactivity

* allow a tag to be renamed and update all notes that use that tag • repurpose RenameFolderModal.styl to RenameModal so it is more generic * call handleConfirmButtonClick directly instead of sending through a confirm method * better name for method to confirm the rename * use close prop instead of a new method * use callback ref instead of legacy string refs * bind the handleChange in the constructor to allow for direct function assignment * update the tag in the URL upon change * use the eventEmitter to update the tags in the SnippetNoteDetail header via the TagSelect component * respect themes when modal is opened * show error message when trying to rename to an existing tag * lint fix, const over let * add missing letter * fix routing and add merge warning dialog * fix space-before-parens lint error * change theming * add check if tag changed Co-authored-by: Khaliq Gant <khaliqgant@gmail.com>

allow a tag to be renamed and update all notes that use that tag

4428963

• repurpose RenameFolderModal.styl to RenameModal so it is more generic

ZeroX-DG added the awaiting review ❇️ Pull request is awaiting a review. label May 4, 2019

ZeroX-DG reviewed May 4, 2019

View reviewed changes

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

khaliqgant added 2 commits May 5, 2019 11:15

call handleConfirmButtonClick directly instead of sending through a c…

242e8b6

…onfirm method

better name for method to confirm the rename

0f6e218

AWolf81 reviewed May 8, 2019

View reviewed changes

khaliqgant added 6 commits May 11, 2019 15:34

Merge branch 'master' of https://github.com/BoostIO/Boostnote into 17…

5873e50

…06-tag-rename

use close prop instead of a new method

e985f12

use callback ref instead of legacy string refs

b1d62c8

bind the handleChange in the constructor to allow for direct function…

ce5d4ac

… assignment

update the tag in the URL upon change

7c5077b

use the eventEmitter to update the tags in the SnippetNoteDetail head…

d2c329a

…er via the TagSelect component

Merge branch 'master' of https://github.com/BoostIO/Boostnote into 17…

e2f0692

…06-tag-rename

khaliqgant added 4 commits September 15, 2019 08:45

Merge branch 'master' of https://github.com/BoostIO/Boostnote into 17…

e493a83

…06-tag-rename

respect themes when modal is opened

9a2dcbc

show error message when trying to rename to an existing tag

4efbe08

lint fix, const over let

c0d22db

Flexo013 requested a review from ZeroX-DG September 16, 2019 08:36

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 Sep 16, 2019

AWolf81 mentioned this pull request Feb 14, 2020

Issue 1706 tag rename - finishing #2989 #3469

Merged

ZeroX-DG closed this Mar 22, 2020

	<ModalEscButton handleEscButtonClick={(e) => this.handleCloseButtonClick(e)} />
	<ModalEscButton handleEscButtonClick={close} />

Uh oh!

Issue 1706: Option to rename tags #2989

Issue 1706: Option to rename tags #2989

Uh oh!

Conversation

khaliqgant commented May 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Issue fixed

Type of changes

Checklist:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AWolf81 commented May 8, 2019

Uh oh!

khaliqgant commented May 11, 2019

Uh oh!

khaliqgant commented May 24, 2019

Uh oh!

khaliqgant commented Jun 13, 2019

Uh oh!

ZeroX-DG commented Jul 27, 2019

Uh oh!

khaliqgant commented Sep 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ZeroX-DG commented Sep 17, 2019

Uh oh!

ZeroX-DG commented Jan 30, 2020

Uh oh!

ZeroX-DG commented Feb 12, 2020

Uh oh!

ZeroX-DG commented Mar 22, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

khaliqgant commented May 3, 2019 •

edited

Loading

khaliqgant commented Sep 15, 2019 •

edited

Loading