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

feat: Add slot to override navbar logo #239

Merged
merged 3 commits into from
Feb 28, 2022
Merged

Conversation

mikeeus
Copy link
Contributor

@mikeeus mikeeus commented Feb 23, 2022

Adds a prop called logo of type NavbarLogo that lets the user override the default logo.
Addresses #238

@mikeeus mikeeus changed the title Add prop to override navbar logo feat: Add prop to override navbar logo Feb 23, 2022
@marinhoarthur
Copy link
Contributor

@mihailmarcu please review.

@mihailmarcu
Copy link
Contributor

mihailmarcu commented Feb 23, 2022

Hey @marinhoarthur, @mikeeus,
The best way to implement such functionality is definitely not props. I think the best solution here is #slots. This will allow us to change not only the logo but also the structure, this solution using props does not allow you to use the svg code directly and manipulate with colors etc.

Also, some developers will want to have different dark theme images or use the figure tag.

@mikeeus
Copy link
Contributor Author

mikeeus commented Feb 23, 2022

@mihailmarcu That's a great idea! I've updated the PR to replace the <router-link> with a named slot called logo.
Is that what you had in mind?

@mikeeus mikeeus changed the title feat: Add prop to override navbar logo feat: Add slot to override navbar logo Feb 23, 2022
Copy link
Contributor

@mihailmarcu mihailmarcu left a comment

Choose a reason for hiding this comment

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

Hey @mikeeus,
Thanks for your input on improving the navigation bar component. The changes look good on our end.

@barriebyron barriebyron added the noteworthy Interesting to share label Feb 28, 2022
@fadeev
Copy link
Contributor

fadeev commented Feb 28, 2022

@marinhoarthur can you, please, review, so we can merge if it looks good. Thanks!

@marinhoarthur marinhoarthur merged commit c62ae54 into ignite:develop Feb 28, 2022
@marinhoarthur
Copy link
Contributor

marinhoarthur commented Feb 28, 2022

closes #238

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
noteworthy Interesting to share
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants