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

Add OrganisationLogo model #4673

Merged
merged 2 commits into from
Mar 5, 2025
Merged

Add OrganisationLogo model #4673

merged 2 commits into from
Mar 5, 2025

Conversation

deborahchua
Copy link
Contributor

@deborahchua deborahchua commented Mar 3, 2025

⚠️ This repo is Continuously Deployed: make sure you follow the guidance ⚠️

What

Add a new OrganisationLogo model which extends Logo model, which takes the organisation's base_path as one of its arguments.

Why

We require the organisation base_path in the organisation_logo publishing component. Not all logos require the base path, so it makes sense to have a separate model that extends the Logo model.
See https://components.publishing.service.gov.uk/component-guide/organisation_logo#how-to-call-this-component

This is part of the app consolidation work and it is in preparation of using this component in various document types.

Trello card

@govuk-ci govuk-ci temporarily deployed to govuk-frontend-app-pr-4673 March 3, 2025 16:42 Inactive
@deborahchua deborahchua marked this pull request as ready for review March 3, 2025 16:46
Copy link
Contributor

@leenagupte leenagupte left a comment

Choose a reason for hiding this comment

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

I have questions about whether this is needed, or if url should be added to the organisation model instead.

@govuk-ci govuk-ci temporarily deployed to govuk-frontend-app-pr-4673 March 5, 2025 11:17 Inactive
We require the organisation `base_path` in the `organisation_logo`
publishing component. Not all logos require the base path, so it
makes sense to have a separate model that extends the Logo model.
See https://components.publishing.service.gov.uk/component-guide/organisation_logo#how-to-call-this-component
@govuk-ci govuk-ci temporarily deployed to govuk-frontend-app-pr-4673 March 5, 2025 12:17 Inactive
@deborahchua deborahchua changed the title Update organisation logo model with base_path Add OrganisationLogo model Mar 5, 2025
Copy link
Contributor

@KludgeKML KludgeKML left a comment

Choose a reason for hiding this comment

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

LGTM

@deborahchua deborahchua merged commit c237308 into main Mar 5, 2025
13 checks passed
@deborahchua deborahchua deleted the update-logo-model branch March 5, 2025 13:37
deborahchua added a commit that referenced this pull request Mar 6, 2025
This was originally added #4673
to accomodate including the `base_path` as the `url` for the
`organisation_logo` publishing component which is not always required
for a logo.

However, we can transform the data required for the component in the
view by passing a hash similar to https://github.com/alphagov/government-frontend/blob/baef09ffcce08cc557a7c01e952824ee803d70f9/app/presenters/content_item/organisation_branding.rb#L8-L13
, so having this additional model is not necessary, and we can
directly access the `base_path` from the `content_item`.

The view will be updated in a separate commit.
deborahchua added a commit that referenced this pull request Mar 6, 2025
This was originally added #4673
to accomodate including the `base_path` as the `url` for the
`organisation_logo` publishing component which is not always required
for a logo.

However, we can transform the data required for the component in the
view by passing a hash similar to https://github.com/alphagov/government-frontend/blob/baef09ffcce08cc557a7c01e952824ee803d70f9/app/presenters/content_item/organisation_branding.rb#L8-L13
, so having this additional model is not necessary, and we can
directly access the `base_path` from the `content_item`.

The view will be updated in a separate commit.
deborahchua added a commit that referenced this pull request Mar 6, 2025
This was originally added #4673
to accomodate including the `base_path` as the `url` for the
`organisation_logo` publishing component which is not always required
for a logo.

However, we can transform the data required for the component in the
view by passing a hash similar to https://github.com/alphagov/government-frontend/blob/baef09ffcce08cc557a7c01e952824ee803d70f9/app/presenters/content_item/organisation_branding.rb#L8-L13
, so having this additional model is not necessary, and we can
directly access the `base_path` from the `content_item`.

The view will be updated in a separate commit.
deborahchua added a commit that referenced this pull request Mar 6, 2025
This was originally added #4673
to accomodate including the `base_path` as the `url` for the
`organisation_logo` publishing component which is not always required
for a logo.

However, we can transform the data required for the component in the
view by passing a hash similar to https://github.com/alphagov/government-frontend/blob/baef09ffcce08cc557a7c01e952824ee803d70f9/app/presenters/content_item/organisation_branding.rb#L8-L13
, so having this additional model is not necessary, and we can
directly access the `base_path` from the `content_item`.

The view will be updated in a separate commit.
deborahchua added a commit that referenced this pull request Mar 6, 2025
This was originally added #4673
to accomodate including the `base_path` as the `url` for the
`organisation_logo` publishing component which is not always required
for a logo.

However, we can transform the data required for the component in the
view by passing a hash similar to https://github.com/alphagov/government-frontend/blob/baef09ffcce08cc557a7c01e952824ee803d70f9/app/presenters/content_item/organisation_branding.rb#L8-L13
, so having this additional model is not necessary, and we can
directly access the `base_path` from the `content_item`.

The view will be updated in a separate commit.
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.

4 participants