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

Remove tag #783

Open
wants to merge 2 commits into
base: master
from

Conversation

@afonsomatos
Copy link

afonsomatos commented Aug 17, 2019

If you want to remove a certain tag from all notes, there's no quick shortcut. This PR adds a menu option for that.

add_remove_tag

Also, I found out this:

updateTagMenu = ( items: MenuItem[] ) => {
this.tag = $(this.ele).data ( 'tag' );
const hasChildren = this.props.container.tag.hasChildren ( this.tag ),
isCollapsed = hasChildren && this.props.container.tag.isCollapsed ( this.tag ),
isCopyable = !Tags.isPrivate ( this.tag );
items[0].visible = hasChildren && !isCollapsed;
items[1].visible = hasChildren && isCollapsed;
items[2].visible = hasChildren && isCopyable;
items[3].visible = isCopyable;
}

Is this a hack to change the items properties dynamically? If so, you could use a getter on those properties, like this:

https://github.com/notable/notable/compare/master...afonsomatos:remove_tag?expand=1#diff-04c912fecf40632f03195cce92314d82R231-R233

Other thing I found tricky is the lack of ESLint. The style of the code is very unusual (lots of whitespaces). Without a linter, it's going to be difficult to maintain a coherent code style.

@CLAassistant

This comment has been minimized.

Copy link

CLAassistant commented Aug 17, 2019

CLA assistant check
All committers have signed the CLA.

@fabiospampinato

This comment has been minimized.

Copy link
Member

fabiospampinato commented Aug 18, 2019

If you want to remove a certain tag from all notes, there's no quick shortcut. This PR adds a menu option for that.

I'm not sure this is a good idea.

The way to do this currently is to:

  1. Select the tag in question
  2. Select all notes
  3. Write the tag to remove in the multi-note editor
  4. Click the button for removing it

Is it too bad? How often does one remove a tag from all notes? I'd prefer not to add unnecessary stuff to the UI otherwise it will eventually become harder to use.

@fabiospampinato

This comment has been minimized.

Copy link
Member

fabiospampinato commented Aug 19, 2019

Is this a hack to change the items properties dynamically?

I don't see it as a hack, we are just updating the menu before displaying it.

If so, you could use a getter on those properties, like this:

Are you sure getters are supported by Electron under that scenario? 🤔

Anyway I like my approach better, all updates are colocated in a single function so they are easier to reason about. Also if 10 menu items all need to call Tags.isPrivate ( this.tag ) or something now we would have to call it 10 times with your approach, or we would have to store that as some kind of global-ish variable. IMHO the current approach is easier to reason about.

@fabiospampinato

This comment has been minimized.

Copy link
Member

fabiospampinato commented Aug 19, 2019

Other thing I found tricky is the lack of ESLint. The style of the code is very unusual (lots of whitespaces). Without a linter, it's going to be difficult to maintain a coherent code style.

Yeah I plan to add it #691, but in the past I spent more time fighting the linter than getting useful infos out of it. So I'm not sure I would necessarily want a linter to dictate the coding style, perhaps a formatter should be used instead.

@afonsomatos

This comment has been minimized.

Copy link
Author

afonsomatos commented Aug 19, 2019

@fabiospampinato The problem is that you are hardcoding the indexes of the items. If you add an extra item in the middle, you'll have unexpected behavior.

@fabiospampinato

This comment has been minimized.

Copy link
Member

fabiospampinato commented Aug 19, 2019

@afonsomatos Yeah that's true. Perhaps we could rebuild the entire menu or do something else less error-prone 🤔

@afonsomatos

This comment has been minimized.

Copy link
Author

afonsomatos commented Aug 19, 2019

@fabiospampinato Here are some ideas (typescript safe):

function makeMenu<T>(
    selector: string,
    items: (source: T) => MenuItemConstructorOptions[],
    source: T | (() => T)) {
        // ...
}


// USAGE:

interface Options {
    isTemplate: boolean,
    isFavorited: boolean,
    isDeleted: boolean
};

function getOptions(): Options {
    return {
        isTemplate: true,
        isFavorited: false,
        isDeleted: true
    }
}

makeMenu(".element", opt => [
    {
        click: () => console.log(opt.isTemplate)
    },
    {
        click: () => console.log(opt.isDeleted)
    },
], getOptions);

// OR MORE DENSED

makeMenu(".element", opt => [
    {
        click: () => console.log(opt.isTemplate)
    }
], {
    isTemplate: true
    }
);

// OR EVEN

makeMenu(".query", ({ isTemplate }) => [
    {
        get visible() {
            return isTemplate;
        }
    }
], {
    get isTemplate(): boolean {
        return Math.random() > 0.5
    }
})

// PRETTIEST

function options() {
    var isTemplate: boolean = Math.random() > 0.5;
    // More logic
    return { isTemplate }
}

makeMenu(".query", ({ isTemplate }) => [
    {
        visible: isTemplate
    }
], options)


@fabiospampinato

This comment has been minimized.

Copy link
Member

fabiospampinato commented Aug 21, 2019

I'm keeping this PR open to collect more feedback about this, but I don't think this should be implemented into the core.

In any case fear not, once plugins #128 are implements it will be possible to implement this as a plugin.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.