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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(button): Add Custom Icons for Custom Buttons (#62) #69

Merged
merged 1 commit into from
Dec 10, 2021

Conversation

Choubakawa
Copy link
Contributor

@Choubakawa Choubakawa commented Dec 9, 2021

Proposed Changes

  • You can add FontAwesome Icon to Custom Button by adding CUSTOM_BUTTON_ICON in the ENV file:
    CUSTOM_BUTTON_ICON=fas link,fab youtube
  • Icons are not required, you can omit an icon in the list or not using the variable at all.
  • This closes Custom Icons for Custom Buttons聽#62

with-all-icons
all icons

not-all-icons
only one

no-icons
without icon

Checklist

  • Tested locally
  • Ran yarn ci to test my code
  • Did not add any unnecessary changes
  • All my changes appear after other changes in each file
  • Included a screenshot (if adding a new button)
  • 馃殌

Copy link
Contributor

@timothystewart6 timothystewart6 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 for this! I made a few comments but nothing blocking!

</div>
);
function App() {
library.add(fab, fas, far);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this can be hoisted outside of the function? I don' think it matters though since app shouldn't re-render

@@ -88,6 +92,7 @@ function Home(props) {
color: textColors[i]?.trim(),
}}
alt={altTexts[i]?.trim()}
icon={icons[i]?.trim()}
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this be

                icon={icons && icons[i]?.trim()}

That way you don't have to have the if statement and null check above and then assign to []? Not a big deal, just a recommendation.

@timothystewart6
Copy link
Contributor

I am totally fine with merging too, just wanted to throw that out there. LMK!

@Choubakawa
Copy link
Contributor Author

Choubakawa commented Dec 10, 2021

I've updated the PR with your recommendations 馃槂

Copy link
Contributor

@timothystewart6 timothystewart6 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!

@timothystewart6 timothystewart6 merged commit a78e0d7 into techno-tim:master Dec 10, 2021
@Choubakawa Choubakawa deleted the custom-icon branch December 10, 2021 17:12
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.

Custom Icons for Custom Buttons
2 participants