Skip to content

add feature: colored tags #2758

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.

Sign up for GitHub

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

Jump to bottom
Merged
Rokt33r merged 7 commits into from
Jan 7, 2019
Merged

add feature: colored tags #2758

Rokt33r merged 7 commits into from
Jan 7, 2019

Conversation

@elfman
Copy link
Contributor

@elfman elfman commented Dec 27, 2018 •
edited
Loading

Description

for #2260.
new feature: colored tags, user can customize the color of tags

ScreenShots

2018-12-28 1 23 38
2018-12-28 1 23 21

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

@elfman elfman force-pushed the colorTag branch 4 times, most recently from 5a911ef to e7d14a6 Compare December 27, 2018 15:05
@KLszTsu
Copy link
Contributor

KLszTsu commented Dec 27, 2018

I think maybe we could use the native color picker of the operating system, as we have used the native right-click menu, tag bar and so on.
I'm sorry I have no idea that how it can be applied; I could only find this, mockingbot/electron-color-picker, hoping it to be helpful.
Thanks.

daiyam
daiyam reviewed Dec 27, 2018
View reviewed changes
browser/components/NoteItem.js Outdated
}

if (showTagsAlphabetically) {
return _.sortBy(tags).map(tag => TagElement({ tagName: tag }))
Copy link
Contributor

@daiyam daiyam Dec 27, 2018

Choose a reason for hiding this comment

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

color: tagConfig[tag] is missing

Copy link
Contributor Author

@elfman elfman Dec 28, 2018

Choose a reason for hiding this comment

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

do you mean a runtime error?

Copy link
Contributor

@daiyam daiyam Dec 28, 2018

Choose a reason for hiding this comment

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

  if (showTagsAlphabetically) {
    return _.sortBy(tags).map(tag => TagElement({ tagName: tag }))
  } else {
    return tags.map(tag => TagElement({ tagName: tag, color: coloredTags[tag] }))
  }

daiyam
daiyam reviewed Dec 27, 2018
View reviewed changes
package.json Outdated
"react": "^15.5.4",
"react-autosuggest": "^9.4.0",
"react-codemirror": "^0.3.0",
"react-color": "^2.17.0",
Copy link
Contributor

@daiyam daiyam Dec 27, 2018

Choose a reason for hiding this comment

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

already in devDependencies

@daiyam
Copy link
Contributor

daiyam commented Dec 27, 2018

@elfman could you use the PR template 😉

Also could you rename your dictionary config.tag with a more explicit name and update the comment of modified functions.

@ZeroX-DG ZeroX-DG added the awaiting review ❇️ Pull request is awaiting a review. label Dec 28, 2018
@ZeroX-DG
Copy link
Member

ZeroX-DG commented Dec 28, 2018 •
edited
Loading

How about something like this. (A quick draw using figma)
frame 2
On white theme:
frame 2 1

@elfman elfman force-pushed the colorTag branch 2 times, most recently from 6c13c5e to 61fa954 Compare December 28, 2018 05:50
@elfman
Copy link
Contributor Author

elfman commented Dec 28, 2018

I have done the requested changes, update PR with template, please review again

2018-12-28 1 23 21

@ZeroX-DG
Copy link
Member

ZeroX-DG commented Dec 28, 2018

@elfman The tag textColor doesn't feel right in the tag. You can use this module: https://github.com/onury/invert-color to generate the tag text color base on the tag background.

@elfman
Copy link
Contributor Author

elfman commented Dec 28, 2018

@ZeroX-DG Updated
2018-12-28 10 14 58

@ZeroX-DG
Copy link
Member

ZeroX-DG commented Dec 29, 2018

Looks nice! But the x symbol is....you get the idea 😄
image

@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 Dec 29, 2018
@elfman
Copy link
Contributor Author

elfman commented Dec 29, 2018

@ZeroX-DG Updated
2018-12-29 10 28 30

ZeroX-DG
ZeroX-DG approved these changes Dec 30, 2018
View reviewed 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.

LGTM 🎉 Looks beautiful

@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 Dec 30, 2018
@daiyam
Copy link
Contributor

daiyam commented Dec 30, 2018

The line 36 of browser/components/NoteItem.js has not been fixed, yet 😉

@ZeroX-DG ZeroX-DG 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 Dec 30, 2018
@ZeroX-DG
Copy link
Member

ZeroX-DG commented Dec 30, 2018

How careless of me! Sorry guys 😢

@elfman
Copy link
Contributor Author

elfman commented Dec 30, 2018

@daiyam @ZeroX-DG fixed, please review again

@daiyam
Copy link
Contributor

daiyam commented Dec 30, 2018

@elfman it's ok for me

ZeroX-DG
ZeroX-DG requested changes Dec 30, 2018
View reviewed changes
browser/components/ColorPicker.styl Outdated
margin-top 2.5px
colorDefaultButton()
border-radius 2px
border $ui-border
Copy link
Member

@ZeroX-DG ZeroX-DG Dec 30, 2018

Choose a reason for hiding this comment

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

It looks better with border: none. Also when I hover the button, its' background is transparent
ab

Copy link
Contributor Author

@elfman elfman Dec 30, 2018

Choose a reason for hiding this comment

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

@ZeroX-DG Updated

ZeroX-DG
ZeroX-DG approved these changes Dec 30, 2018
View reviewed 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.

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 Dec 30, 2018
@Rokt33r Rokt33r added next release (v0.11.13) and removed approved 👍 Pull request has been approved by sufficient reviewers. labels Jan 7, 2019
@Rokt33r Rokt33r merged commit fe011e8 into BoostIO:master Jan 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ZeroX-DG ZeroX-DG ZeroX-DG approved these changes

+1 more reviewer

@daiyam daiyam daiyam left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@elfman @KLszTsu @daiyam @ZeroX-DG @Rokt33r