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: Allows Logos to be exported, and adds ability to customise title #701

Merged
merged 6 commits into from
Jan 31, 2025

Conversation

curlyfriesplease
Copy link
Contributor

@curlyfriesplease curlyfriesplease commented Jan 30, 2025

PR description

What is it doing?

Allows 'Logos' to be used in the frontend as it's not currently an export, and also allows a custom 'title' prop

Why is this required?

So that the logo can be used across multiple headers in various repos

link to Jira ticket:

DF-205

<Logos animateRotate sizeSm="50px" sizeMd="60px" />
Copy link
Contributor

Choose a reason for hiding this comment

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

We've previously had logos rotated a bit (by default, not on hover), so would rather we don't make this PR destructive in that respect

Copy link
Contributor

Choose a reason for hiding this comment

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

transform: ${props => (props.rotate ? 'rotate(-14deg)' : 'inherit')};

Copy link
Contributor

Choose a reason for hiding this comment

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

Although I guess this is just an example, so doesn't matter, now I thiink about it for 10 seconds

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I wasn't sure about it, opted for it just to help show off the possible functionality within the CL, I reckon if there is functionality that's not immediately obvious it's always good to enable it, for the purposes of demoing it in your CL showcase thingy

Copy link
Contributor

Choose a reason for hiding this comment

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

Could always add a second example that covers this functionality, leaving the previous one in place (with some descriptive titles above); makes for even-clearer explanation of what we have and how to use it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good shout. I've got two examples in place instead:

image

Copy link
Contributor

Choose a reason for hiding this comment

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

Lovely stuff

Copy link
Contributor

@AndyEPhipps AndyEPhipps left a comment

Choose a reason for hiding this comment

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

Looking good man; just now a bit confused why we ever had Logo and Logos in the first place 🤔

Looks like Logos is pretty specific to the header nav, and it sometimes includes more than one Logo.. but always contains a link.

Move to rename Logos to LogosWithLink or something, to delineate?

@curlyfriesplease
Copy link
Contributor Author

Looking good man; just now a bit confused why we ever had Logo and Logos in the first place 🤔

Looks like Logos is pretty specific to the header nav, and it sometimes includes more than one Logo.. but always contains a link.

Move to rename Logos to LogosWithLink or something, to delineate?

I agree. Changing now...

@curlyfriesplease curlyfriesplease merged commit f6aa54b into master Jan 31, 2025
9 checks passed
@curlyfriesplease curlyfriesplease deleted the DF-205-logos-export branch January 31, 2025 11:32
Copy link

🎉 This PR is included in version 8.14.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@AndyEPhipps
Copy link
Contributor

AndyEPhipps commented Jan 31, 2025

I'll spin up a CRcom PR to bring in these changes, updating the usage accordingly

Edit: didn't even need to; no direct usage

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

Successfully merging this pull request may close these issues.

2 participants