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

follows: refactor FollowButton.js #1780

Merged
merged 1 commit into from
Mar 10, 2025
Merged

follows: refactor FollowButton.js #1780

merged 1 commit into from
Mar 10, 2025

Conversation

hom3mad3
Copy link
Contributor

@hom3mad3 hom3mad3 commented Mar 6, 2025

Description
Refactor FollowButton to allow for alerts outside the container

Copy link
Collaborator

@vellip vellip left a comment

Choose a reason for hiding this comment

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

Thank you! Looks good already, I just left some general questions. Also, should we add some tests?

return (
<span className="a4-follow">
<span className={`a4-follow ${useMbStyles ? 'a4-follow--proposal-style' : ''}`}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of asking for specific projects, can we not add like a className prop or customClassName or similar and then you pass a4-follow--proposal-style in MB as a prop

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh good catch! that was unintended, i forgot to change it!

const buttonClasses = following ? 'a4-btn a4-btn--following' : 'a4-btn a4-btn--follow'

const AlertPortal = () => {
if (!alert || !alertTarget) return null
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this actually be && or ||?

Comment on lines 74 to 75
const container = document.getElementById(alertTarget)
if (!container) return null
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe add a console.error or throw an error, I think that's better for devs to understand where the issue is then.

@hom3mad3 hom3mad3 force-pushed the mr-2025-03-fix-follow branch 2 times, most recently from 009715a to 8f018fb Compare March 10, 2025 11:35
@hom3mad3 hom3mad3 force-pushed the mr-2025-03-fix-follow branch from 8f018fb to 3f25bd4 Compare March 10, 2025 11:44
@hom3mad3 hom3mad3 merged commit 1b17f7f into main Mar 10, 2025
2 checks passed
@hom3mad3 hom3mad3 deleted the mr-2025-03-fix-follow branch March 10, 2025 11:49
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