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

Desktop: Fixes #3569: Show full folder name in mouse-over pop-up text #3590

Merged

Conversation

Jumanjii
Copy link
Contributor

@Jumanjii Jumanjii commented Aug 3, 2020

fixes #3569

Defining the tooltip property here, it is then propagated to the ToolbarButton component that will use it to define the html title of the tag while keeping the truncated version as content.

const tooltip = this.props.tooltip ? this.props.tooltip : title;

https://github.com/laurent22/joplin/blob/dev/ElectronClient/gui/ToolbarButton.jsx#L11

joplin-hover

Copy link
Owner

@laurent22 laurent22 left a comment

Choose a reason for hiding this comment

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

Looks good but please remove the comma changes. It can cause unnecessary conflicts.

Comment on lines 52 to 56
cmdService.commandToToolbarButton('historyBackward')
cmdService.commandToToolbarButton('historyBackward'),
);

output.push(
cmdService.commandToToolbarButton('historyForward')
cmdService.commandToToolbarButton('historyForward'),
Copy link
Owner

Choose a reason for hiding this comment

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

Please remove these changes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad, I will update it tonight.
I thought it was the linter hook that added it but it was probably just my editor 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, so it was really the husky linter hook that added them and to push without them as you recommend,
I had to temporarily delete the hook from the package.json.

Totally new on the project so tell me if I should have use another easier solution 😸

Running npm lint from the root of the repository to test, it updated 65 files.
Did I miss a configuration somewhere?

@Jumanjii Jumanjii force-pushed the fix-hover-foldername-search-result branch from b280b6c to b3f0273 Compare August 4, 2020 17:43
@laurent22
Copy link
Owner

Thanks for the update. Indeed we have a rule for this so I didn't understand why would the linter change the code for you but not for me or other developers.

It seems the reason is that eslint messed up an update and passed a breaking change as a minor version: eslint/eslint#13166 So probably you got the minor version update and I'm still on the previous version. So in the end your change was correct and once I update eslint too, I'll apply the update the whole code base.

Anyway, your main change is good so let's merge!

@laurent22 laurent22 merged commit e4cfb51 into laurent22:dev Aug 4, 2020
@Jumanjii Jumanjii deleted the fix-hover-foldername-search-result branch August 18, 2020 14:22
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.

Show full folder name in mouse-over pop-up text
2 participants