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.

Sign up for GitHub

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

Jump to bottom

feat: logo slot and hide_logo attribute added to footer #2611

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from
Draft

feat: logo slot and hide_logo attribute added to footer #2611

wants to merge 7 commits into from

Conversation

marianayovcheva
Copy link
Contributor

@marianayovcheva marianayovcheva commented Jan 17, 2023 •
edited

While working on recent footer updates, I needed to change the logo in the footer and later remove the logo completely. I thought that it would be good to add more flexibility to the design system regarding the footer logo. This PR:

  • adds the ability to remove the logo using the hide_logo attribute
  • adds logo slot to allow customising the logo where needed. If there is no logo slot added, it defaults to the current logo link (otherwise this would be a breaking change)

The way the logo slot works is inspired by the HeaderComponent logo slot

@marianayovcheva marianayovcheva added the docs preview If this is added a preview version of the docs site will be deployed label Jan 17, 2023
@github-actions
Copy link

🚀 Netlify deployed citizens-advice-design-system as draft

https://63c6c4a61de77f573bfcdc38--citizens-advice-design-system.netlify.app

@marianayovcheva marianayovcheva marked this pull request as ready for review January 17, 2023 15:56
davidsauntson
davidsauntson reviewed Jan 19, 2023
View reviewed changes
Copy link
Contributor

@davidsauntson davidsauntson left a comment •
edited

Choose a reason for hiding this comment

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

This is great!

I have a couple of thoughts that it would be good to get your's and @davidrapson's views on.

  • we could rip the band aid off and have a breaking change where the presence of the logo slot or not determines if it is rendered, rather than a dedicated boolean param
  • we could infer the presence of the logo from the homepage_url and not provide a fallback for it here

    design-system/engine/app/components/citizens_advice_components/footer.rb

    Line 14 in afe2cd9

    @homepage_url = homepage_url || "/"
    - this wouldn't be a breaking change and make a little bit of sense to me, since the url is only ever rendered in the logo anyway
  • or we could do this, which is also fine but wondering if we can avoid an additional param to keep the interface simpler

@davidrapson
Copy link
Contributor

I definitely prefer this version:

we could rip the band aid off and have a breaking change where the presence of the logo slot or not determines if it is rendered, rather than a dedicated boolean param

Especially as it's a closer match for the header component then. I think we can be relatively bullish on breaking changes to component interfaces given if we need to be. Supporting hide_logo feels most awkward to me, would be happy with the homepage_url option too.

General approach looks ace though, always happy to avoid needing custom css background images for this use case.

@github-actions
Copy link

🚀 Netlify deployed citizens-advice-design-system as draft

https://63d00a19ef57fb10d437bd15--citizens-advice-design-system.netlify.app

@github-actions
Copy link

🚀 Netlify deployed citizens-advice-design-system as draft

https://63d00d5aa4e676009fd866e6--citizens-advice-design-system.netlify.app

@marianayovcheva marianayovcheva mentioned this pull request Jan 24, 2023
Open
5 tasks
@marianayovcheva
Copy link
Contributor Author

Thanks both! I've now added the footer logo slot as a breaking change. I've also ended up removing the homepage_url attribute because it was only used by the previous logo. I've mentioned this feature in: #2626

marianayovcheva
marianayovcheva commented Jan 24, 2023
View reviewed changes
demo/app/components/app_footer_component.rb Outdated
@@ -3,6 +3,7 @@
class AppFooterComponent < ViewComponent::Base
def call
render CitizensAdviceComponents::Footer.new do |c|
c.logo(url: "/")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we've discussed it before but it feels like the logo in the design system docs should lead to the main CA site (or be a design system specific logo)

@marianayovcheva marianayovcheva mentioned this pull request Apr 20, 2023
Merged
@davidrapson davidrapson reopened this Aug 21, 2023
davidrapson
davidrapson approved these changes Aug 22, 2023
View reviewed changes
Copy link
Contributor

@davidrapson davidrapson left a comment •
edited

Choose a reason for hiding this comment

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

👍 Realised this was still waiting on review but it looks fab to me barring the older conflicts from the viewcomponent 3 upgrade.

@marianayovcheva marianayovcheva marked this pull request as draft August 25, 2023 15:30
@github-actions
Copy link

🚀 Netlify deployed citizens-advice-design-system as draft

https://64e8d2264e0fc50d2243b630--citizens-advice-design-system.netlify.app

@github-actions
Copy link

🚀 Netlify deployed citizens-advice-design-system as draft

https://6509aca1de9fe8062150d96d--citizens-advice-design-system.netlify.app

@github-actions
Copy link

🚀 Netlify deployed citizens-advice-design-system as draft

https://6509afee63b6e30877956d5f--citizens-advice-design-system.netlify.app

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidsauntson davidsauntson davidsauntson left review comments

@davidrapson davidrapson davidrapson approved these changes

Assignees
No one assigned
Labels
docs preview If this is added a preview version of the docs site will be deployed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@marianayovcheva @davidrapson @davidsauntson