Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

refactor: move required external auth buttons to the submit side#18586

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Open
BrunoQuaresma wants to merge5 commits intomain
base:main
Choose a base branch
Loading
frombq/improve-external-auth-on-tasks

Conversation

BrunoQuaresma
Copy link
Collaborator

Before:
Screenshot 2025-06-25 at 14 40 16

After:
Screenshot 2025-06-25 at 14 53 53

Copy link
Member

@code-ashercode-asher left a comment

Choose a reason for hiding this comment

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

I think it would make sense to make the new component have parity with the previous one (or make the previous one reusable) but it looks good to me otherwise.

<Spinnerloading={isLoadingExternalAuth}>
<ExternalImagesrc={auth.display_icon}/>
</Spinner>
Login with{auth.display_name}
Copy link
Member

Choose a reason for hiding this comment

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

I still find the "Login with $name" buttons confusing without context, if it was "Connect $name" I think it would be clearer, but that could just be me. What do you think?

That would also makes better sense with theSlack Notify external auth,Connect Slack Notify makes more sense to me thanLogin with Slack Notify.

BrunoQuaresma reacted with thumbs up emoji
Copy link
CollaboratorAuthor

Choose a reason for hiding this comment

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

Makes sense. Wdyt about the "Connect to $name"?

Copy link
Member

Choose a reason for hiding this comment

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

I like it!

startPollingExternalAuth();
}}
>
<Spinnerloading={isLoadingExternalAuth}>
Copy link
Member

Choose a reason for hiding this comment

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

Before, we were showing a spinner while polling (externalAuthPollingState === "polling"), but now we only show while loading. Just want to confirm if that was intentional. I do think it makes sense to have some visual indication when the polling is happening.

If it was intentional, we can drop the spinner altogether;isLoadingExternalAuth will always be false here right? Since if we are loading, we have no auth buttons to display.

Copy link
CollaboratorAuthor

@BrunoQuaresmaBrunoQuaresmaJun 26, 2025
edited
Loading

Choose a reason for hiding this comment

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

Thanks for catching this! definitely a bug.

variant="outline"
key={auth.id}
size="sm"
disabled={isLoadingExternalAuth||auth.authenticated}
Copy link
Member

Choose a reason for hiding this comment

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

isLoadingExternalAuth will always be false here, I think. Also, before we were disabling the buttons while polling. Want to check if dropping that was intentional.

Copy link
CollaboratorAuthor

Choose a reason for hiding this comment

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

Same as previous, a bug! 🐛

missedExternalAuth:TemplateVersionExternalAuth[];
};

constExternalAuthButtons:FC<ExternalAuthButtonProps>=({
Copy link
Member

Choose a reason for hiding this comment

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

This does not handle the abandoned polling state like the previous component does. It seems important to me otherwise you could be waiting for it to connect but nothing ever happens, if the polling was abandoned, with no visual indication as to why or a way to retry (aside from refreshing the browser).

Would it make sense to modify the other component to have a variant so we can reuse it here? Or would it make more sense to duplicate the retry code maybe?

Copy link
CollaboratorAuthor

@BrunoQuaresmaBrunoQuaresmaJun 26, 2025
edited
Loading

Choose a reason for hiding this comment

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

Another good catch! I will implement it.

Until we have to repeat this 3x I'm in favor of duplication if it is does not lead to a painful maintenance in the future. Maybe we could move some of these reusable logic to theuseExternalAuth hook.

code-asher reacted with thumbs up emoji
Copy link
CollaboratorAuthor

@BrunoQuaresmaBrunoQuaresmaJun 26, 2025
edited
Loading

Choose a reason for hiding this comment

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

Thinking more on this: When the user abadon, or does not complete the flow, the button gets back to its regular state right? So why would we need to handle this in a special way when the user can just click in the button again?

Copy link
Member

Choose a reason for hiding this comment

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

It could be that they had to leave their desk or do something for a bit and finished linking their account after one minute. So not actually abandoned, it just took longer than the poll timeout.

A temporary network issue could also result in a similar state, I think.

In these cases we could just let it go back to the regular state, but the user needs to click the button again, and IMO we should be explicit about that rather than wait for the user to figure it out. I could see myself sitting there waiting for the account to link wondering if the dashboard is broken 😆

Copy link
CollaboratorAuthor

Choose a reason for hiding this comment

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

Hm... ok 👍

Run task
</Button>
<divclassName="flex items-center gap-2">
{missedExternalAuth&&isMissingExternalAuth&&(
Copy link
Member

Choose a reason for hiding this comment

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

isMissingExternalAuth has no effect here, right? It is already implied bymissedExternalAuth and because the component only renders something ifmissedExternalAuth.length > 0.

BrunoQuaresma reacted with thumbs up emoji
Copy link
CollaboratorAuthor

@BrunoQuaresmaBrunoQuaresmaJun 26, 2025
edited
Loading

Choose a reason for hiding this comment

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

I think so! Lets see what TS thinks about your logic.

code-asher reacted with heart emoji
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@hugodutkahugodutkaAwaiting requested review from hugodutka

@code-ashercode-asherAwaiting requested review from code-asher

Assignees

@BrunoQuaresmaBrunoQuaresma

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

2 participants
@BrunoQuaresma@code-asher

[8]ページ先頭

©2009-2025 Movatter.jp