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

QuickEditor: Missing action on gravatar icon #400

Merged
merged 5 commits into from
Oct 18, 2024

Conversation

mlumeau
Copy link
Member

@mlumeau mlumeau commented Oct 18, 2024

Closes #376

Description

Adds a redirection to https://gravatar.com/profile in a custom browser tab when clicking on the bottomsheet's Gravatar icon in the top right corner.

edit: launches in a regular browser tab to align with the behavior of the "view profile" button.

Testing Steps

  • While logged out of Gravatar in your browser, click on the icon and see that you are redirected to the login page
  • While logged in Gravatar in your browser, click on the icon and see that you are redirected to your profile edition page

@mlumeau mlumeau added this to the 2.1.0 milestone Oct 18, 2024
@mlumeau mlumeau added bug Something isn't working enhancement Enhancement and removed bug Something isn't working labels Oct 18, 2024
@mlumeau mlumeau self-assigned this Oct 18, 2024
@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Oct 18, 2024

📲 You can test the changes from this Pull Request in Gravatar Demo by scanning the QR code below to install the corresponding build.
App Name Gravatar Demo
Commit6050124
Direct Downloadgravatar-demo-prototype-build-pr400-6050124.apk

Copy link
Contributor

@AdamGrzybkowski AdamGrzybkowski left a comment

Choose a reason for hiding this comment

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

Adds a redirection to https://gravatar.com/profile in a custom browser

I don't think the current code will use the custom browser tab. It will launch the browser. Launcing the custom tab looks more or less like this:

val customTabIntent: CustomTabsIntent = CustomTabsIntent.Builder()
        .build()
customTabIntent.launchUrl(context, URL)

I've also left a comment in the Issue on how to show the specific profile rather /profile. This could show the wrong profile if the user is logged in with a different email then used in the QE.

@@ -24,7 +26,9 @@ import com.gravatar.quickeditor.R
import com.gravatar.ui.GravatarTheme

@Composable
internal fun QETopBar(onDoneClick: () -> Unit, modifier: Modifier = Modifier) {
internal fun QETopBar(onDoneClick: () -> Unit, gravatarIconUrl: String, modifier: Modifier = Modifier) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
internal fun QETopBar(onDoneClick: () -> Unit, gravatarIconUrl: String, modifier: Modifier = Modifier) {
internal fun QETopBar(onDoneClick: () -> Unit, onGravatarIconClick: () -> Unit = {}, modifier: Modifier = Modifier) {

I would expose the onGravatarIconClick rather than pass the URL to let the parent composable decide what behavior they want on icon tap. I think it's more flexible, WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in 187258d

@mlumeau
Copy link
Member Author

mlumeau commented Oct 18, 2024

I don't think the current code will use the custom browser tab

You're right, I had that in mind but then decided to go with a regular tab in the code to do the same as the "view profile" button, forgot to change that in the PR description.

@hamorillo
Copy link
Contributor

I've also left a comment in the Issue on how to show the specific profile rather /profile. This could show the wrong profile if the user is logged in with a different email then used in the QE.

This is important. I think we'll need to "launch" the callback at least to GravatarQuickEditorBottomSheet and, from there, open the profile using the email from gravatarQuickEditorParams.

@mlumeau
Copy link
Member Author

mlumeau commented Oct 18, 2024

This is important. I think we'll need to "launch" the callback at least to GravatarQuickEditorBottomSheet and, from there, open the profile using the email from gravatarQuickEditorParams

In that case if the email is not associated to a gravatar account it will 404. That's what I did in my first commit. Also if the user is signed in a different email there will be an error message in the bottom sheet after oauth, so I think it's enough.

@AdamGrzybkowski
Copy link
Contributor

If you think that's a better URL to open I think it's best to discuss with iOS folks to make sure both platforms are aligned.

Copy link
Contributor

@AdamGrzybkowski AdamGrzybkowski left a comment

Choose a reason for hiding this comment

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

:shipit:

@mlumeau
Copy link
Member Author

mlumeau commented Oct 18, 2024

Permission to merge into the release branch? They're targeting this release on iOS.

@mlumeau mlumeau merged commit 881711b into release/2.0.0 Oct 18, 2024
18 of 19 checks passed
@mlumeau mlumeau deleted the mlumeau/376-missing-action-Gravatar-icon branch October 18, 2024 13:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

QuickEditor: Missing action on the Gravatar icon
4 participants