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

New header component for landing pages #3942

Merged
merged 29 commits into from
May 9, 2024
Merged

New header component for landing pages #3942

merged 29 commits into from
May 9, 2024

Conversation

jenhadfield-dfe
Copy link
Contributor

Trello card - https://trello.com/c/EXASvrqj/5830-build-new-header-component

Context

Changes proposed in this pull request

Guidance to review

@github-actions github-actions bot added ruby Pull requests that update Ruby code test Style content Content changes labels Apr 10, 2024
@gemmadallmandfe
Copy link
Contributor

@jenhadfield-dfe Just checking you have seen the build failure on this PR?

Might be one for @martyn-w as it mentions Shakapacker....

@jenhadfield-dfe
Copy link
Contributor Author

jenhadfield-dfe commented May 1, 2024 •
edited
Loading

thanks @gemmadallmandfe , it was the deleted image ttalianne.jpg. I've put it back as it looks like the image name is mentioned in some other files that I'm unfamiliar with, so might be better for @martyn-w to remove the image separately

@gemmadallmandfe

This comment was marked as resolved.

Sign in to view
@gemmadallmandfe
Copy link
Contributor

@jenhadfield-dfe I've had a look through all the pages again - looks good

The only issue I can see is on a couple of pages where the descender on the g is overlapped (plus the on campus colours - see separate comment)

image

image

However, I am happy for us to merge this PR and address this issue as a lower priority issue afterwards if that would help - I'm keen that we start getting the value from these improved headers sooner rather than later!

@jenhadfield-dfe jenhadfield-dfe requested review from martyn-w and gemmadallmandfe and removed request for MylesJarvis May 9, 2024 11:05
gemmadallmandfe
gemmadallmandfe previously approved these changes May 9, 2024
View reviewed changes
Copy link
Contributor

@gemmadallmandfe gemmadallmandfe left a comment

Choose a reason for hiding this comment

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

Looks good to me

martyn-w
martyn-w approved these changes May 9, 2024
View reviewed changes
Copy link
Contributor

@martyn-w martyn-w left a comment

Choose a reason for hiding this comment

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

👍 LGTM

@github-actions GitHub Actions
Copy link

github-actions bot commented May 9, 2024

Review app deployed to https://get-into-teaching-app-review-3942.test.teacherservices.cloud

@sonarqubecloud SonarQubeCloud
Copy link

sonarqubecloud bot commented May 9, 2024

Quality Gate Passed Quality Gate passed

Issues
8 New issues
10 Accepted issues

Measures
0 Security Hotspots
96.2% Coverage on New Code
0.2% Duplication on New Code

See analysis details on SonarCloud

@jenhadfield-dfe jenhadfield-dfe merged commit 17d0534 into master May 9, 2024
26 checks passed
@jenhadfield-dfe jenhadfield-dfe deleted the new-header-component branch May 9, 2024 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@martyn-w martyn-w martyn-w approved these changes

@gemmadallmandfe gemmadallmandfe gemmadallmandfe left review comments

Assignees
No one assigned
Labels
Config content Content changes Review ruby Pull requests that update Ruby code Style test
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jenhadfield-dfe @gemmadallmandfe @martyn-w @mariosh-e