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 FXIOS-10009 [Bookmarks Evolution] Update Bookmarks Panel View Edit Mode Part 2 #23926

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

DanielDervishi
Copy link
Collaborator

@DanielDervishi DanielDervishi commented Dec 20, 2024

📜 Tickets

Jira ticket
Github issue

💡 Description

  • Added the ability to undo bookmark deletion for bookmarks/folders
  • Updated edit affordances
  • Added drag and drop into folders for bookmarks/folders

📝 Checklist

You have to check all boxes before merging

  • Filled in the above information (tickets numbers and description of your work)
  • Updated the PR name to follow our PR naming guidelines
  • Wrote unit tests and/or ensured the tests suite is passing
  • When working on UI, I checked and implemented accessibility (minimum Dynamic Text and VoiceOver)
  • If needed, I updated documentation / comments for complex code and public methods
  • If needed, added a backport comment (example @Mergifyio backport release/v120)

Copy link
Contributor

@lmarceau lmarceau left a comment

Choose a reason for hiding this comment

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

Bunch of nits related to self. usage (which is part of our Swift style guide), and indentation.

Some other comments are bit more of importance, let me know if I can provide further information on any of those!

@DanielDervishi DanielDervishi force-pushed the DD/10008_update-bookmark-panel-edit-p2 branch from 4aaedd7 to c11f7b3 Compare December 23, 2024 18:49
Copy link
Contributor

@lmarceau lmarceau left a comment

Choose a reason for hiding this comment

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

LGTM!

@MattLichtenstein
Copy link
Collaborator

MattLichtenstein commented Dec 23, 2024

A couple of observations from a implementation / usability perspective:

  • Deleting the recent bookmarks folder and then undoing the deletion doesn't reset the folder as the recent bookmarks folder location again. Do you think it should?

  • At some font sizes, there is not much, if any spacing between the bookmark/folder title and the "edit" action button in the undo toast when the title is long

Screenshot 2024-12-23 at 2 33 49 PM
  • When font size is large, the "Undo" button in the toast has a weird border that I don't see on other toasts with action buttons
Screenshot 2024-12-23 at 2 33 28 PM

@mobiletest-ci-bot
Copy link

Warnings
⚠️ Deferred class seems to be used in file firefox-ios/Client/Frontend/Library/Bookmarks/Utitlity/BookmarksSaver.swift at line 95. Please consider replacing with completion handler instead.
Messages
📖 Project coverage: 34.28%
📖 Edited 10 files
📖 Created 0 files

Client.app: Coverage: 32.44

File Coverage
HistoryPanel.swift 34.76% ⚠️
LocalDesktopFolder.swift 37.5% ⚠️
BookmarksViewController.swift 9.65% ⚠️
OneLineTableViewCell.swift 0.0% ⚠️
ActionToast.swift 0.0% ⚠️
BookmarksFolderCell.swift 0.0% ⚠️
BookmarksSaver.swift 97.95%

libStorage.a: Coverage: 56.51

File Coverage
RustPlaces.swift 80.79%

Generated by 🚫 Danger Swift against c11f7b3

@DanielDervishi
Copy link
Collaborator Author

  • At some font sizes, there is not much, if any spacing between the bookmark/folder title and the "edit" action button in the undo toast when the title is long

A couple of observations from a implementation / usability perspective:

* Deleting the recent bookmarks folder and then undoing the deletion doesn't reset the folder as the recent bookmarks folder location again. Do you think it should?

* At some font sizes, there is not much, if any spacing between the bookmark/folder title and the "edit" action button in the undo toast when the title is long
Screenshot 2024-12-23 at 2 33 49 PM
* When font size is large, the "Undo" button in the toast has a weird border that I don't see on other toasts with action buttons
Screenshot 2024-12-23 at 2 33 28 PM

Thanks Matt! Yeah definitely it should restore the recently saved folder. Theoretically if someone is pressing undo its because they've changed their mind about deletion and would like to return to the previous bookmarks state.

Also I'll take a look at the undo toast. I was using a pre-made component so I sort of assumed that it would be alright in terms of scaling.

@DanielDervishi DanielDervishi force-pushed the DD/10008_update-bookmark-panel-edit-p2 branch from c11f7b3 to 9d07d43 Compare December 23, 2024 21:00
Copy link
Contributor

mergify bot commented Dec 23, 2024

This pull request has conflicts when rebasing. Could you fix it @DanielDervishi? 🙏

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.

4 participants