-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Privacy mode: instead of blurring, use an illegible font (#3376) #3377
Privacy mode: instead of blurring, use an illegible font (#3376) #3377
Conversation
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
a78736f
to
49b25c0
Compare
Bundle Stats — desktop-clientHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset
View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger No assets were bigger Smaller
Unchanged
|
Bundle Stats — loot-coreHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset No files were changed View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger No assets were bigger Smaller No assets were smaller Unchanged
|
49b25c0
to
7f01cd7
Compare
This looks cool :) I wonder if moving the privacy filter to wrap the text instead of the entire component would fix the shifting |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found the convention for using class
es. I thought Actual was inline style
only 😅 That opened doors. The layout shift is gone.
actual-privacy-mode-redacted-font-2.mov
style: { | ||
position: 'absolute', | ||
width: '100%', | ||
height: '100%', | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This appears to be safe to remove. But I'm also not sure why it's in current master
. So I may be missing some edge case not covered by the visual tests, and this might need to be reverted.
packages/desktop-client/src/components/budget/report/ReportComponents.tsx
Outdated
Show resolved
Hide resolved
[ | ||
{ | ||
display: 'inline-flex', | ||
position: 'relative', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potentially breaking. I didn't notice any instances where PrivacyFilter content is positioned relative to something higher up the DOM. Doesn't break the visual tests, but I don't know how completely those cover the app. If someone else knows that this will be a problem, lmk
cbd9de3
to
f8e0530
Compare
For the breaking test I'd like help from someone more experienced with this project. "Budget › transfer funds to another category" fails for this PR in GitHub Actions but passes for me locally. Fwiw, locally I've seen "Onboarding > creates a new empty budget file" and "Accounts › Budgeted Accounts > creates a transfer from two existing transactions" be flaky. Per https://github.com/actualbudget/actual/blob/master/packages/desktop-client/README.md#visual-regression, to run the tests locally I'm doing |
f8e0530
to
b21f57f
Compare
Have you tried |
Okay I did
and then in another terminal once on
Based on my browser testing and the changeset, I expect all tests to pass. |
We're getting there :) You need to push the new screenshots that were updated due to the |
👋 Hi! It looks like this PR has not had any changes for a week now. Would you like someone to review this PR? If so - please remove the "[WIP]" prefix from the PR title. That will let the community know that this PR is open for a review. |
b21f57f
to
1a774d4
Compare
That's odd. @MatissJanis Any idea why would the screenshots show up as added instead of modified? |
Heh that's why I hadn't added them before. Note that none of them include private mode text either. |
I'm guessing the VRT command got ran outside the docker container (on a local machine). Please remove them and re-run the VRT tests inside docker. Here are the instructions: https://github.com/actualbudget/actual/blob/master/packages/desktop-client/README.md |
@MatissJanis I ran |
Perfect. Then undo all the screenshot changes you did here and re-run that command once more. The output will be updates to screenshots. |
@coderabbitai full review |
@MikesGlitch: Understood, I will perform a full review of the pull request. Actions performedFull review triggered. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- packages/desktop-client/src/components/PrivacyFilter.tsx (2 hunks)
Additional comments not posted (5)
packages/desktop-client/src/components/PrivacyFilter.tsx (5)
8-9
: LGTM: Import ofcss
from 'glamor'The addition of the
css
import from 'glamor' is appropriate for the new styling approach used in the refactored component. This change enables more dynamic and flexible CSS-in-JS styling.
67-67
: LGTM: Updated PrivacyFilter to use PrivacyOverlayThe change from
BlurredOverlay
toPrivacyOverlay
is consistent with the PR objectives to replace the blurring effect with a more privacy-focused approach using an illegible font.
102-111
: LGTM: Styling of the first child divThe styling of the first child div using glamor is appropriate. The use of flex display and flex-grow ensures that the content fills the available space correctly.
Line range hint
41-43
: LGTM: Removal of blurIntensity propThe removal of the
blurIntensity
prop fromPrivacyFilterProps
is appropriate given the shift from a blur effect to using an illegible font. This change simplifies the component's interface and aligns with the new privacy implementation.
81-81
: Verify potential breaking change in positioningAs noted in a previous comment, there's a potential for breaking changes related to positioning of PrivacyFilter content relative to higher DOM elements. While visual tests have passed, it's crucial to thoroughly investigate this across the application.
To ensure there are no unexpected layout shifts or positioning issues, please run the following verification steps:
Review the output to identify any instances where PrivacyFilter is used in a context sensitive to positioning. Pay special attention to cases where it's nested within other positioned elements or used with specific layout techniques.
Verification successful
Potential breaking change in positioning: Verified
After reviewing all instances of the
PrivacyFilter
component, no positioning-related properties (position
,top
,left
,bottom
,right
,transform
) were found. This confirms that there are no potential breaking changes related to the positioning ofPrivacyFilter
within the application.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for all usages of PrivacyFilter and analyze their context # Step 1: Find all files using PrivacyFilter echo "Files using PrivacyFilter:" rg --type typescript --type tsx "PrivacyFilter" # Step 2: Analyze the context of PrivacyFilter usage echo "\nContext of PrivacyFilter usage:" rg --type typescript --type tsx -C 5 "PrivacyFilter" # Step 3: Look for potential positioning-related props or styles echo "\nPotential positioning-related usage:" rg --type typescript --type tsx -C 5 "PrivacyFilter.*position|top|left|bottom|right|transform"Length of output: 529
Script:
#!/bin/bash # Description: Search for all usages of PrivacyFilter and analyze their context # Step 1: Find all files using PrivacyFilter echo "Files using PrivacyFilter:" rg "PrivacyFilter" --glob "*.ts" --glob "*.tsx" # Step 2: Analyze the context of PrivacyFilter usage echo "\nContext of PrivacyFilter usage:" rg -C 5 "PrivacyFilter" --glob "*.ts" --glob "*.tsx" # Step 3: Look for potential positioning-related props or styles echo "\nPotential positioning-related usage:" rg "PrivacyFilter.*(position|top|left|bottom|right|transform)" --glob "*.ts" --glob "*.tsx"Length of output: 71627
Updated @joel-jeremy
|
@olets would be great to get this into the next release (start of October), would you mind having a look at the merge conflict please and then we'll get a review in? |
packages/desktop-client/src/components/budget/tracking/TrackingBudgetComponents.tsx
Outdated
Show resolved
Hide resolved
b4005db
to
a1d0dce
Compare
c3a0833
to
134180d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is excellent work. LGTM!
I'll wait for another review since others have given feedback.
That second-to-last CI run was "fixed" by |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thank you for your efforts on this!
Instead of blurring, uses the font Redacted Script https://github.com/christiannaths/redacted-font#redacted-script.
The good: much more private
Edit: the following limitation no longer applies (see #3377 (review))
The touchy: this font's sizing isn't identical to the non-private font. In some cases there's a minor layout shift on hover, and/or when toggling between modes.The implementation support per-elementline-height
customization, and could easily be extended to support per-elementletter-spacing
. I haven't gone deep down that rabbit hole, only customizing line height on the two large font size numbers, where the difference is more significant.screenshot
screencap, updated
actual-privacy-mode-redacted-font-2.mov
OLD outdated screencapture, for posterity
actual-privacy-mode-redacted-font.mov