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

CMS View Component Tidy #2266

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

Conversation

A-Wheeto
Copy link
Contributor

Moved into CMS namespace
Updated tests
Updated previews
Refactored code where needed

Status

  • Current Status: WIP / Ready for (front-end / tech) review
  • Review App link(s): populate this with links to the relevant parts of the review app
  • Closes add issue number or delete
  • Resolves add issue number, resolves add another issue number (or delete)
  • Fixes add external repo (org/repo) or delete
  • Related to add issue numbers or delete

Review progress:

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

What's changed?

Description of what's been done - bullets are usually best, but you do you.

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.

@tc-deploybot tc-deploybot temporarily deployed to teachcomputing-pr-2266 January 20, 2025 14:22 Inactive
@A-Wheeto A-Wheeto temporarily deployed to teachcomputing-pr-2266 January 21, 2025 15:10 Inactive
@tc-deploybot tc-deploybot temporarily deployed to teachcomputing-pr-2266 January 28, 2025 12:52 Inactive
Copy link
Contributor

@msquance-stem msquance-stem left a comment

Choose a reason for hiding this comment

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

LGTM, one little thing that might be worth looking at but not essential.

I would suggest we hold off merging this in till after primary goes live, as easier to fix any conflicts in here, than the other way round I suspect

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it worth adding in preview for video and image in here too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great idea, I will add this into a preview.

@A-Wheeto A-Wheeto temporarily deployed to teachcomputing-pr-2266 January 28, 2025 13:56 Inactive
Moved into CMS namespace
Updated tests
Updated previews
Refactored code where needed
Refactor previews and tests
Create files for missing previews
Tidy up exisiting previews
Fix broken previews
Remove empty connect method
Remove duplicate CSS class
@A-Wheeto A-Wheeto force-pushed the 2933-cms-viewcomponent-tidy-up branch from abb8169 to cbead70 Compare February 3, 2025 14:34
Copy link

sonarqubecloud bot commented Feb 3, 2025

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