Skip to content

Move footer to Strapi #2425

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Move footer to Strapi #2425

wants to merge 6 commits into from

Conversation

A-Wheeto
Copy link
Contributor

@A-Wheeto A-Wheeto commented May 16, 2025

Status

Review progress:

  • Browser tested
  • Front-end review completed
  • Tech review completed

What's changed?

  • Replaced existing footer with CMS Footer component

Steps to perform after deploying to production

If the production environment requires any extra work after this PR has been deployed detail it here. This could be running a Rake task, migrating a DB table, or upgrading a Gem. That kind of thing.

A-Wheeto added 4 commits May 16, 2025 07:23
…ated a new view component

Basic styling and layout on the component completed
Initial setup of mocks and testing
Updating the mock
Adding stub into system and request tests
Updating footer testing
@tc-deploybot tc-deploybot temporarily deployed to teachcomputing-pr-2425 May 30, 2025 14:00 Inactive
Copy link

@markjs markjs left a comment

Choose a reason for hiding this comment

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

A couple of comments on some of the lookups in the component, otherwise looks good 👍

end

def company_logo
data_models_by_type(Cms::Models::Images::Image).first
Copy link

Choose a reason for hiding this comment

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

Seems a bit risky fetching these values based on their order. It looks like there's a key being used elsewhere, perhaps this could be refactored to make use of the key here too instead of first/second?

data.select { |model| model.is_a?(type) }
end

def find_text_field(index)
Copy link

Choose a reason for hiding this comment

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

This method seems a little unnecessary. The company_url and funding_url methods could be rewritten to be more similar to the company_logo and funding_logo methods to just have the data_models_by_type call directly in them and this extracted method wouldn't be needed.

…s by key

Refactored footer component
Updated and added additional tests
@A-Wheeto A-Wheeto temporarily deployed to teachcomputing-pr-2425 June 4, 2025 13:50 Inactive
@A-Wheeto A-Wheeto temporarily deployed to teachcomputing-pr-2425 June 4, 2025 13:59 Inactive
Copy link

sonarqubecloud bot commented Jun 4, 2025

@markjs
Copy link

markjs commented Jun 19, 2025

@A-Wheeto I don't love the way the view now has a bunch of get_model calls. Especially don't love that some of them then have .render or .value on the end. Feels like the view is now a bit of a mess and the interface to the component values isn't obvious.

How about using the get_model method internally from within the component class but keeping the individual data pieces as methods. So the component would have something like

def company_logo_link
  get_model(:company_logo_link).value
end

def company_logo
  render get_model(:company_logo).render
end

and then your view file can just call each of these methods in place and look nice and tidy.

I'm not sure on having the render call at the start and end of the company_logo method as per my example, still may need to have render company_logo in the view itself but you can try out and see what works.

What do you think?

@A-Wheeto
Copy link
Contributor Author

Thanks @markjs - on second look you have a good point on the get_model calls in the view. I'll tidy this up.

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.

3 participants