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

Adjust frontend styles to be compatible with latest core themes #187

Merged
merged 2 commits into from
Apr 12, 2024

Conversation

claytoncollie
Copy link
Contributor

Description of the Change

This PR addresses styling issues found on the latest Core theme and WordPress version in issue #138

Some of the issues I did not find and others I felt did not need to be addressed.

I tested on Twenty Twenty Four, Twenty Twenty Three, Twenty Twenty Two, and Twenty Twenty on WordPress 6.5

1 - Spacing issue on 'Audience Profiles' Section

I did not see this issue and no changes were made.

image

2 - Spacing issue on 'Why you should choose digital' Section

I adjusted the top margin for this group as well as the interior padding on all four sides. I did not place the headings and icons inline as that can be done with a Row block in the editor.

image

3 - Font is different in all section

The font faces were all correct with what is being loaded on the site. The font sizes and color looked intentional in the stylesheets as well. I did not change anything related to this issue.

image

4 - Background color is missing in 'Our package' and 'Why you should choose digital' section

Our Packages does not have a background color nor does it have one in the CSS file that is not being targetted properly. I did not change this section.

Why Choose does have a background color and was left as-is. Look at the screenshot above in number 2. It is a very faint background color but it is there.

image

5 - Background colour is missing in table block

The even rows in the table do not have a background color or a text color the way odd rows do. Even though the existing styles look good on all tested themes, I went ahead and added a background color and text color based on the core present CSS variables just in case these colors are wildly different than the background color. To the naked eye, nothing looks to have changed.

image

6 - Still Have Questions Background Color

This section does have a background color although it is very faint. I did not change this section.

image

Closes #183

How to test the Change

  1. Activate the plugin
  2. View the Media Kit page on the frontend
  3. Review styles

Changelog Entry

Added - Margin to the top of Why Digital Group
Added - Padding to Why Digital Group
Added - Background and text color to even table rows

Credits

Props @claytoncollie

Checklist:

  • I agree to follow this project's Code of Conduct.
  • [] I have updated the documentation accordingly.
  • [] I have added tests to cover my change.
  • [] All new and existing tests pass.

@github-actions github-actions bot added this to the 1.3.4 milestone Apr 3, 2024
@github-actions github-actions bot added the needs:code-review This requires code review. label Apr 3, 2024
@jeffpaul jeffpaul requested review from faisal-alvi and removed request for dkotter and jeffpaul April 4, 2024 15:46
Copy link
Member

@faisal-alvi faisal-alvi left a comment

Choose a reason for hiding this comment

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

@claytoncollie Thanks for your efforts! E2E Test failures seem due to the artifact version and will be fixed in #182.

Also, it would be great to make the changes that were skipeed? Maybe in a separate PR?

@claytoncollie
Copy link
Contributor Author

@faisal-alvi I am happy to make the changes that were skipped but I will need some direction on what to change them to.

  1. Skipped due to the issue not being present. Spacing was correct when I installed the plugin.
  2. Changes were made from feedback
  3. Skipped due to lack of direction. QA points out that fonts are different but I will need direction on what they should be changed to.
  4. Skipped due to lack of direction. QA points out that a background color is not present. I will need design direction on this one. The section looks pretty good to me.
  5. Changes were made from feedback.
  6. Skipped due to background color being present already. The color is very faint but it is there.

@faisal-alvi
Copy link
Member

faisal-alvi commented Apr 8, 2024

@claytoncollie these screenshots show the desired design. I think achieving this design in all the TTx theme series is difficult (for example, the background color), is it? If it is, we may have to brainstorm over design tweaks that are capable in all TTx themes. cc @jeffpaul

@jeffpaul jeffpaul merged commit 0926109 into develop Apr 12, 2024
9 of 12 checks passed
@jeffpaul jeffpaul deleted the fix/frontend-styles branch April 12, 2024 18:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs:code-review This requires code review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Frontend Issues
3 participants