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

fix bookmark editing and colors #364

Merged
merged 6 commits into from
Dec 10, 2021
Merged

fix bookmark editing and colors #364

merged 6 commits into from
Dec 10, 2021

Conversation

brindy
Copy link
Contributor

@brindy brindy commented Dec 7, 2021

Task/Issue URL: https://app.asana.com/0/1199230911884351/1201476225858250
Tech Design URL:
CC:

Description:

Editing got difficult with the state changes that were added. This PR fixes the colors for all the icons, the URL text and makes the editing state behave sensibly.

Steps to test this PR:

Please check in both light and dark mode.

  1. Check selection states work as expected
  2. Edit a bookmark
  3. Ensure clicking on a different bookmark ends the edit state

Testing checklist:

  • Dark Mode
  • Light Mode
Internal references:

Software Engineering Expectations
Technical Design Template
When ready for review, remember to post the PR in MM

@brindy
Copy link
Contributor Author

brindy commented Dec 7, 2021

@bstandaert-ddg - I'll get someone else to code review as well, but feel free to have a play with this.

@samsymons would you mind reviewing the code, please?

@bstandaert-ddg
Copy link

This is looking a lot better, but selecting the text in the title and URL fields still isn't working consistently for me:

Screen.Recording.2021-12-07.at.3.13.23.PM.mov

@samsymons samsymons self-assigned this Dec 7, 2021
@brindy
Copy link
Contributor Author

brindy commented Dec 8, 2021

Yeah seems to be a matter of clicking and then waiting for it to react. Will see if I can fix that. thanks @bstandaert-ddg

@brindy
Copy link
Contributor Author

brindy commented Dec 9, 2021

OK, root cause is removing this:

tableView.selectionHighlightStyle = .none

Which is what makes selections work 🤦

Gonna see if I can work around it.

@brindy
Copy link
Contributor Author

brindy commented Dec 9, 2021

Fixed! 😅

Can you give it another whirl, please @bstandaert-ddg

@samsymons - I think this is ready for review now.

@samsymons samsymons self-requested a review December 9, 2021 16:30
@bstandaert-ddg
Copy link

That worked! The UI looks good to me now.

I'm still not able to edit a bookmark URL, because then I run into this pre-existing bug: https://app.asana.com/0/1199178362774117/1201222157355618/f. But editing the title works now at least.

samsymons
samsymons approved these changes Dec 9, 2021
View reviewed changes
Copy link
Collaborator

@samsymons samsymons left a comment

Choose a reason for hiding this comment

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

No issues here, nice work on this Brindy 👏

@brindy
Copy link
Contributor Author

brindy commented Dec 10, 2021

Thanks @bstandaert-ddg and @samsymons :)

@brindy brindy merged commit 9f9dfd2 into develop Dec 10, 2021
@brindy brindy deleted the brindy/bookmark-editing branch December 10, 2021 01:01
samsymons added a commit that referenced this pull request Dec 13, 2021
@samsymons
Merge branch 'develop' into sam/mac-waitlist-lock-screen
f390b98 
# By Alexey Martemyanov (3) and others
# Via GitHub
* develop:
  Point to updated BSK version (#371)
  Fixing macOS desktop app retention metrics (#370)
  fix bookmark editing and colors (#364)
  ATB -> PixelDataStore (#356)
  Fix deallocation (#369)
  Initialize user scripts when setting up the WebView (#363)
  bookmarks management improvements (#362)
  BSK branch setting changed to exact commit (#360)
  Tweak the alignment of the home page search bar. (#353)

# Conflicts:
#	DuckDuckGo.xcodeproj/project.pbxproj
#	DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved
samsymons added a commit that referenced this pull request Dec 17, 2021
@samsymons
Merge branch 'develop' into sam/enable-identity-autofill
e94fa81 
# By Alexey Martemyanov (4) and others
# Via GitHub (1) and Tomas Strba (1)
* develop:
  mvp onboarding (#377)
  Update dark mode shield icon asset (#374)
  Mac Waitlist Lock Screen (#372)
  drop legacy atb (#376)
  Version 0.18.0
  Hotfix: Rename ATB keys so that the app thinks it's a new install (#375)
  Version 0.17.8
  Point to updated BSK version (#371)
  Fixing macOS desktop app retention metrics (#370)
  fix bookmark editing and colors (#364)
  ATB -> PixelDataStore (#356)
  Fix deallocation (#369)

# Conflicts:
#	DuckDuckGo.xcodeproj/project.pbxproj
#	DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@samsymons samsymons samsymons approved these changes

Assignees

@samsymons samsymons

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@brindy @bstandaert-ddg @samsymons