Skip to content
This repository has been archived by the owner on Feb 24, 2025. It is now read-only.

Zoom improvements #3810

Merged
merged 16 commits into from
Feb 12, 2025
Merged

Zoom improvements #3810

merged 16 commits into from
Feb 12, 2025

Conversation

mallexxx
Copy link
Collaborator

@mallexxx mallexxx commented Jan 31, 2025

Task/Issue URL: https://app.asana.com/0/1201048563534612/1208866166089091/f

Description:

  • Add icons to …->Zoom menu
  • Update (deblur) Zoom icon
  • Adjust button design; add separator
  • https://www.figma.com/design/Qc38lLo707Q92vtpRxwVnR/%F0%9F%94%8D-Page-Zoom?node-id=7-19895&m=dev
  • Display "–" icon when zoomed out
  • Update Permissions buttons design (add corner radius)
  • Clicking “Zoom In” from the menu increases the zoom level, closes the menu, and displays the contextual zoom UI.
  • The contextual zoom UI should close automatically after a 4-second delay if no interaction happens.
  • Popover arrow misplaced when zooming in local file (and the Zoom button happens to be leftmost item in the Address Bar)
  • Popover stays on screen when switching tab (with ctrl+Tab)

Optional E2E tests:

  • Run PIR E2E tests
    Check this to run the Personal Information Removal end to end tests. If updating CCF, or any PIR related code, tick this.

Steps to test this PR:

  1. Validate icons are present in the menu,
  2. cmd+➕/➖/0️⃣ should work, popover shown on menu items click or hotkey
  3. popover should hide after [value set in Debug->Zoom UI->Menu->Interval] when mouse is out; popover should hide after [value set in Debug->Zoom UI->Toolbar->Interval] when opened using the button in the address bar and not closed when mouse is hovering over the popover or the button
  4. – should be displayed when zoomed out
  5. permission buttons should work (https://permission.site) with updated design
  6. separator should be displayed when both Shield and Zoom/Permission buttons are displayed
  7. Zoom UI popover shouldn‘t be misplaced when zooming in a local html or image file
  8. the popover should close when switching tabs (ctrl+Tab)

Definition of Done:


Internal references:

Pull Request Review Checklist
Software Engineering Expectations
Technical Design Template
Pull Request Documentation

Copy link
Contributor

github-actions bot commented Jan 31, 2025

Warnings
⚠️ PR has more than 500 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by 🚫 dangerJS against cadb938

Copy link
Collaborator

@SabrinaTardio SabrinaTardio left a comment

Choose a reason for hiding this comment

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

Works as expected!
Assuming that testing step 3 is slightly wrong:

  • popover should hide after 4s when mouse is out; popover shouldn‘t close when opened using the button in the address bar

-> It actually closes after 2 seconds from the address bar button

I left a few comments / questions

@@ -16,47 +16,10 @@
// limitations under the License.
//

import SwiftUI
import AppKit
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am assuming we moved away from SwiftUI for positioning etc problems?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yup, I couldn‘t make it look like on the design using SwiftUI

static let toolbarCloseIntervalItem = NSUserInterfaceItemIdentifier("toolbarCloseIntervalItem")
}

final class ZoomPopoverDebugMenu: NSMenu {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this to allow reviewer to check which interval works best?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, to be removed before merging

Copy link
Collaborator

@SabrinaTardio SabrinaTardio left a comment

Choose a reason for hiding this comment

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

Thank you! Looks great!

@mallexxx mallexxx merged commit d24591d into main Feb 12, 2025
23 checks passed
@mallexxx mallexxx deleted the alex/zoom-improvements branch February 12, 2025 12:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants