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

Add tag colors setup #704

Merged
merged 1 commit into from
Jan 3, 2021

Conversation

Komediruzecki
Copy link
Contributor

@Komediruzecki Komediruzecki commented Nov 30, 2020

Add tag colors setup (#323):

  • Add tag context menu (right click on tag) for chosing colors
  • Add react color as color pickers library
  • Add basic DB management for tags with color (FSNote, PouchDB, createStore API)
  • Add basic UI component (react-color) for choosing the color
  • Add basic handling of color brightness in tags and hover styles (black/white inversion, brigther, dimmer when bg is dark/light)
  • Add colored tags inside note list note tag list

Fix right click context menu in sidebar

How it works:
Users can right click on any tag in note tag list. The color picker comes up and on selection it changes colors (preview is below in color slider at right corner), below that are some predefined colors which can be clicked. Once user is satisfied with color, to close the dialog any click outside of the color picker will do. The color is then updated in UI acordingly.

For background colors which are too dark, hovering the tag will brighten it up, opposite works too.
For background colors which are too dark, text and remove tag button are light/white and vice versa.

The following image shows picking color:
InstructionsTagsChosingColor

Some colored tags preview:
ColoredTagsNotePreview

ColoredTagsNotePreview_Colored

v0.11.5 - new tag location examples:
ColorChoser
ColorList

Test:

  • In electron Linux App (dev)
  • In electron Linux App production version (appImage)

IssueHunt Summary

Referenced issues

This pull request has been submitted to:


@Komediruzecki Komediruzecki force-pushed the feature/add-colored-tags branch 2 times, most recently from a989841 to fb181c1 Compare December 5, 2020 09:30
@Komediruzecki Komediruzecki self-assigned this Dec 5, 2020
const [colorPickerModal, showColorPickerModal] = useState(false)

const openTagContextMenu: React.MouseEventHandler = useCallback(
(_) => {
Copy link
Member

Choose a reason for hiding this comment

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

If you don't need to access the event parameter, you don't need to provide the handler type

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed!

title: 'Cannot update tag color.',
description: 'Invalid note or tag.',
})
console.log('Tag name or note was null', 'requested color:', color)
Copy link
Member

Choose a reason for hiding this comment

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

Delete this!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed!

@@ -220,12 +220,18 @@ const NotePage = ({ storage }: NotePageProps) => {

const { showSearchModal } = useSearchModal()

const storageTags = useMemo(() => {
if (storage == null) return []
return values(storage.tagMap).map((tag) => tag)
Copy link
Member

Choose a reason for hiding this comment

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

[...storage.tagMap.values()]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no values() on ObjectMap

So final should be:
return [...values(storage.tagMap)]? copy of tags
or maybe even
return values(storage.tagMap)? raw values

@@ -77,11 +78,17 @@ const WikiNotePage = ({ storage }: WikiNotePageProps) => {

const { showSearchModal } = useSearchModal()

const storageTags = useMemo(() => {
if (storage == null) return []
return values(storage.tagMap).map((tag) => tag)
Copy link
Member

Choose a reason for hiding this comment

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

[...storage.tagMap.values()]

Copy link
Contributor Author

@Komediruzecki Komediruzecki Dec 9, 2020

Choose a reason for hiding this comment

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

There is no values() on ObjectMap

So final should be:
return [...values(storage.tagMap)]? copy of tags
or maybe even
return values(storage.tagMap)? raw values


const brightnessDefaultThreshold = 110

export function getColorBrightness(color: RGBColor | string | null) {
Copy link
Member

Choose a reason for hiding this comment

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

Don't accept null

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

}

export function isColorBright(
color: RGBColor | string | null,
Copy link
Member

Choose a reason for hiding this comment

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

Dont accept null

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

@@ -922,6 +928,7 @@ export function createDbStoreCreator(
return {
...(await targetStorage.db.getTag(tagName))!,
name: tagName,
color: '',
Copy link
Member

Choose a reason for hiding this comment

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

If color is not set, I'd like to leave it as undefined

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed!

map[name] = {
...tagDoc,
name,
color: tagDoc.color || '',
Copy link
Member

Choose a reason for hiding this comment

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

Don't fill empty string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

@@ -70,6 +70,7 @@ export type TagDoc = {
} & TagDocEditibleProps

export type TagDocEditibleProps = {
color?: string
data: JsonObject
Copy link
Member

Choose a reason for hiding this comment

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

I'd like to put it in data like bookmarked prop of NoteDoc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a bit more work, because it is not a boolean.
But I think I managed to do it.

@@ -199,3 +204,14 @@ export const flexCenter = () => `display: flex;
align-items: center;
justify-content: center;
`

export const tagBgColor = ({ theme, color }: StyledProps & TagStyleProps) => `
Copy link
Member

Choose a reason for hiding this comment

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

tagBackgroundColor

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed!

@Rokt33r Rokt33r added the awaiting changes 🖊️ Pull request has been reviewed, but contributor needs to make changes. label Dec 6, 2020
@Komediruzecki Komediruzecki force-pushed the feature/add-colored-tags branch 2 times, most recently from d48ce37 to f8ac689 Compare December 9, 2020 16:28
@Komediruzecki Komediruzecki 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 Dec 9, 2020
map[name] = {
...tagDoc,
name,
color: tagColor,
Copy link
Member

Choose a reason for hiding this comment

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

We can access color directly from tagDoc.data.color I don't see any benifits

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, a mistake! Fixed now!

@@ -118,6 +118,7 @@ export type PopulatedFolderDoc = FolderDoc & {

export type PopulatedTagDoc = TagDoc & {
name: string
color?: string
Copy link
Member

Choose a reason for hiding this comment

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

Should be discarded

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, correct. Done!

@Rokt33r Rokt33r 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 14, 2020
@Komediruzecki Komediruzecki 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 Dec 15, 2020
@guneskaan
Copy link
Contributor

@Rokt33r, @Komediruzecki: One concern I have with this is that the web browser users will not be able to right click on tags and open the context menu, as they can only perform left click. Instead, you can add a control to the SideNavigatorItem, where the users can click on the three vertical dots to open the context menu (similar to note and folder context menus).

I've implemented this in #730. Feel free to copy over the code, or change the implementation.

@Komediruzecki
Copy link
Contributor Author

Komediruzecki commented Dec 20, 2020

@Rokt33r, @Komediruzecki: One concern I have with this is that the web browser users will not be able to right click on tags and open the context menu, as they can only perform left click. Instead, you can add a control to the SideNavigatorItem, where the users can click on the three vertical dots to open the context menu (similar to note and folder context menus).

I've implemented this in #730. Feel free to copy over the code, or change the implementation.

Hi,

I agree that it will be a problem for users who don't have the typical right-click option (I suppose you mean mobile browser users or native android version in general?)

I looked over the PR and I am missing some images how all of this looks like, can you share them?
Also, if there are three dots next to the tags list, which tag will be renamed? it should be possible for any tag? If so, this might be a bit trickier than adding such control (or I might be mistaken if this is something else, maybe a picture would help to visualize it).

If the implementation wise makes sense, sure I will transfer the code or rebase it onto it if it merges to master sooner.

@guneskaan
Copy link
Contributor

I should've attached some pictures to make it clear, my bad. The web browser app (www.boostnote.io/app) will not allow users to right click. This is not the case only for mobile users, but for desktop browser users as well. This is what they'll see when they right click:
Screen Shot 2020-12-21 at 10 11 52 AM
Screen Shot 2020-12-21 at 10 12 24 AM

By adding a control option (three vertical dots) to the list view, we can allow them to view the context menu by left clicking on this button:
Screen Shot 2020-12-21 at 10 13 43 AM

This control option may not be possible in the note view due to lack of space. But even then, I believe we should give the user the ability to use this feature through the list view

@Komediruzecki
Copy link
Contributor Author

I should've attached some pictures to make it clear, my bad. The web browser app (www.boostnote.io/app) will not allow users to right click. This is not the case only for mobile users, but for desktop browser users as well. This is what they'll see when they right click:
Screen Shot 2020-12-21 at 10 11 52 AM
Screen Shot 2020-12-21 at 10 12 24 AM

By adding a control option (three vertical dots) to the list view, we can allow them to view the context menu by left clicking on this button:
Screen Shot 2020-12-21 at 10 13 43 AM

This control option may not be possible in the note view due to lack of space. But even then, I believe we should give the user the ability to use this feature through the list view

Oh, I see, you are right. I'll rebase and integrate changes to make it easier for those users too. Thanks for the suggestion.

},
[tag, updateTagColorByName]
)
const tagColor = useCallback(() => {
Copy link
Member

Choose a reason for hiding this comment

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

Use useMemo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Used!

colorBrightness: number
}

export default class DialogColorPicker extends React.Component<
Copy link
Member

Choose a reason for hiding this comment

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

I don't see any benefits to make a class component. Please covert this into a function component!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, refactored

Add tag context menu (right click on tag) for chosing colors
Add react color as color pickers library
Add basic DB management for tags with color (FSNote, PouchDB, createStore API)
Add basic UI component (react-color) for choosing the color
Add basic handling of color brightness in tags and hover styles (black/white inversion, brigther, dimmer when bg is dark/light)
Add colored tags inside note list note tag list
@Rokt33r Rokt33r merged commit 94732ec into BoostIO:master Jan 3, 2021
@Rokt33r Rokt33r added this to the v0.13 milestone Jan 3, 2021
@Komediruzecki Komediruzecki removed the awaiting review ❇️ Pull request is awaiting a review. label Jan 6, 2021
@Komediruzecki Komediruzecki mentioned this pull request Jan 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants