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

Make website URLs clickable in Logins+ #462

Merged
merged 1 commit into from
Mar 10, 2022
Merged

Conversation

ayoy
Copy link
Collaborator

@ayoy ayoy commented Mar 10, 2022

Task/Issue URL: https://app.asana.com/0/1177771139624306/1201548487133814/f
Tech Design URL:
CC:

Description:
If domain can represent a valid URL, display it as a text button that opens a website in a new tab, otherwise fall back to a text label. Opening a website does not cause Logins+ popover to hide, which I think is a feature, as it still may be useful for the user.

Steps to test this PR:

  1. Open Logins+ with at least 1 saved website password
  2. Verify that "Website URL" is clickable and opens the website in a new tab
  3. Add a new login item with an invalid URL.
  4. Verify that "Website URL" is not clickable.

Testing checklist:

  • Test with Release configuration

Internal references:

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

@ayoy ayoy force-pushed the dominik/logins-clickable-links branch from 79271ef to 99cf391 Compare March 10, 2022 11:50
@brindy brindy self-assigned this Mar 10, 2022
Copy link
Contributor

@brindy brindy left a comment

Choose a reason for hiding this comment

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

LGTM! One minor thing to change related to the color for the link, then feel free to merge.

model.openURL(domainURL)
} label: {
Text(model.domain)
.foregroundColor(.accentColor)
Copy link
Contributor

Choose a reason for hiding this comment

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

For the link color there's a color in the asset catalog called "LinkBlueColor" which we should use here. We do use accent for most things normally though so I understand why you chose that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, thanks! Indeed, I found accentColor being used to highlight selected item in the Logins+ items list. Will update this as you suggest 👍

@brindy brindy assigned ayoy and unassigned brindy Mar 10, 2022
@ayoy ayoy force-pushed the dominik/logins-clickable-links branch from 99cf391 to 3627cf3 Compare March 10, 2022 16:01
@ayoy ayoy merged commit b931621 into develop Mar 10, 2022
@ayoy ayoy deleted the dominik/logins-clickable-links branch March 10, 2022 16:24
samsymons added a commit that referenced this pull request Mar 10, 2022
* develop:
  New Tab Page (#433)
  Make website URLs clickable in Logins+ (#462)
  Correction of URL if it is missing one slash '/' (#459)
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.

2 participants