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.

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

Add 'View on Hover' to Category Notes for #563 #579

Merged
merged 6 commits into from
Jan 27, 2023
Merged

Add 'View on Hover' to Category Notes for #563 #579

merged 6 commits into from
Jan 27, 2023

Conversation

gsumpster
Copy link
Contributor

Closes #563

This PR adds a hover to the NotesButton that displays NotesTooltip!

@netlify
Copy link

netlify bot commented Jan 27, 2023 •
edited
Loading

✅ Deploy Preview for actualbudget ready!

Name Link
🔨 Latest commit 74ce13f
🔍 Latest deploy log https://app.netlify.com/sites/actualbudget/deploys/63d4392c68e32a000875425e
😎 Deploy Preview https://deploy-preview-579--actualbudget.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

j-f1
j-f1 reviewed Jan 27, 2023
View reviewed changes
Copy link
Contributor

@j-f1 j-f1 left a comment

Choose a reason for hiding this comment

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

Thanks for opening the PR! Trying it out in the preview, it looks really great. I did notice a couple of small UI issues, though:

  • the spacing between the text and the edge of the tooltip is slightly larger than between the texture and the edge of the tooltip. This isn’t a huge issue but it would be nice to fix if possible.
  • Newlines aren’t handed properly (due to how HTML renders whitespace). Textareas default to white-space: pre-wrap, and the preview text should set this too.

@Kidglove57
Copy link

Kidglove57 commented Jan 27, 2023 •
edited
Loading

Thanks again @gsumpster. I am loving this! But I agree, it would be great to get new lines displaying correctly.

Thought - is there any merit in giving the "per month" budget (and/or accounts)notes the same hover treatment?

@gsumpster
Copy link
Contributor Author

Thanks for reviewing @j-f1, great catch on the new lines, didn't think to test that! That should be fixed now. I've also added a touch of padding to the textarea, I felt it looked a bit nicer with some padding but if there are strong opinions here, I can make both the textarea and the text itself have no padding.

@Kidglove57, this component is shared across both of those locations, so this change will also apply to those automatically 🙌

@MatissJanis
Copy link
Member

👋 Nice! Would you mind adding word-break too?

Screenshot 2023-01-27 at 20 22 35

@gsumpster
Copy link
Contributor Author

Done!

@MatissJanis
Copy link
Member

Screenshot 2023-01-27 at 20 33 59

break-word would be better used here. Instead of break-all

@gsumpster
Copy link
Contributor Author

break-word would be better used here. Instead of break-all

We love text wrapping 🙈 – I avoided break-word because it's deprecated, but it looks like overflow-wrap: break-word does the same thing here, so switched that out!

MatissJanis
MatissJanis approved these changes Jan 27, 2023
View reviewed changes
Copy link
Member

@MatissJanis MatissJanis 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 to me! I'll leave it with Jed to give the final approval since he was involved here too.

@MatissJanis
Copy link
Member

break-word would be better used here. Instead of break-all

We love text wrapping 🙈 – I avoided break-word because it's deprecated, but it looks like overflow-wrap: break-word does the same thing here, so switched that out!

Thanks! And hah - you made me feel like an old dinosaur there 🦕 . I did not know this css attribute was deprecated.

j-f1
j-f1 approved these changes Jan 27, 2023
View reviewed changes
Copy link
Contributor

@j-f1 j-f1 left a comment

Choose a reason for hiding this comment

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

LGTM!

@j-f1 j-f1 merged commit 72006ef into actualbudget:master Jan 27, 2023
@trafico-bot trafico-bot bot added ✨ Merged Pull Request has been merged successfully and removed ✅ Approved labels Jan 27, 2023
FlorianLang06 pushed a commit to FlorianLang06/actual that referenced this pull request Mar 7, 2024
@gsumpster @j-f1
Add 'View on Hover' to Category Notes for actualbudget#563 (actualbud…
ee292fb 
…get#579)

* add hover state to NotesButton

* switch NotesButton editability to a ternary

* fix new lines, adjust padding on textarea

* add workBreak to text styling

* Update packages/loot-design/src/components/NotesButton.js

Co-authored-by: Jed Fox <[email protected]>

* change wordBreak to overflowWrap

---------

Co-authored-by: Jed Fox <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@j-f1 j-f1 j-f1 approved these changes

@MatissJanis MatissJanis MatissJanis approved these changes

Assignees
No one assigned
Labels
✨ Merged Pull Request has been merged successfully
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Feature] Category Notes - Reveal on hover
4 participants
@gsumpster @Kidglove57 @MatissJanis @j-f1