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

Term home 110 #234

Open
wants to merge 19 commits into
base: trunk
Choose a base branch
from
Open

Term home 110 #234

wants to merge 19 commits into from

Conversation

dballari
Copy link

@dballari dballari commented Sep 6, 2024

This PR contains work from issue #110
The responsive behaviour is still not done. I'm looking for the way to stack the grid elements one on top of the other, but I still do not know how.

To test this PR, activate Term theme and have at least 10 posts on your WordPress. Then visit the home page.
Screenshot from 2024-09-06 10-49-49

@dballari
Copy link
Author

Hi, I tried to send this PR and it is giving errors.

I think it might have something to do with the fact that I cann't run an npm command in my development environment. If I try npm install, I get this message 'sh: 1: husky: not found'. Could any one help me?

I'm using a local by flywheel on linux.

node v20.12.2, npm v10.7.0 and running npm install from the site shell provided by local.

PHP v8.3.0 and web server nginx

@MaggieCabrera
Copy link
Collaborator

Hi, I tried to send this PR and it is giving errors.

I think it might have something to do with the fact that I cann't run an npm command in my development environment. If I try npm install, I get this message 'sh: 1: husky: not found'. Could any one help me?

I'm using a local by flywheel on linux.

node v20.12.2, npm v10.7.0 and running npm install from the site shell provided by local.

PHP v8.3.0 and web server nginx

That's really strange, I use nvm (the .nvmrc file is saying we should use 20.10.0) and I have

Now using node v20.10.0 (npm v10.2.3)

This let me do an npm install. I suggest you try cloning the repo again, it's probably going to be the easiest way to debug

'label' => __( 'Hover effect', 'term'),
'inline_style' => '
.is-style-hover-effect {
background-color: #EDECE8;
Copy link
Collaborator

Choose a reason for hiding this comment

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

this gives a really nice effect but it has other side effects, namely:

  • If the user decides to change the original color palette (or if we decide to add additional styles, which is likely), the hover effect may no longer match the new colors
  • If the user adds new elements/blocks like a post date or similar, we no longer cover that possibility.

@MaggieCabrera
Copy link
Collaborator

I made some changes to this PR to make it look closer to the design. I still have problems getting the grid to behave correctly:

If we set the grid to manual, that means the column numbers will always be the same regardless of viewport size. If we set it to automatic, we can't make it look good most of the time. I'm thinking of a way to do this maybe using CSS and container queries. I'll come back to this PR, this is an interesting challenge.

@dballari
Copy link
Author

I made some changes to this PR to make it look closer to the design. I still have problems getting the grid to behave correctly:

Yes it looks closer to the design

If we set the grid to manual, that means the column numbers will always be the same regardless of viewport size. If we set it to automatic, we can't make it look good most of the time. I'm thinking of a way to do this maybe using CSS and container queries. I'll come back to this PR, this is an interesting challenge.

Yes, container queries are not often used but they may address the issue

  • If the user decides to change the original color palette (or if we decide to add additional styles, which is likely), the hover effect may no longer match the new colors

  • If the user adds new elements/blocks like a post date or similar, we no longer cover that possibility.

I don't see why new element would not behave ok, since the group with the style variation groups everything together. But I agree to remove it for the moment and deal with the responsive behavior which is more important.
I will try to think a little about it and If I get some results I'll get back here

It's true it's an interesting challenge

@MaggieCabrera
Copy link
Collaborator

oh, btw, the screenshot needs updating. Also, the requirements for the directory say:

The screenshot must not be bigger than 1200 x 900px
The ratio of width to height needs to be 4:3

@dballari
Copy link
Author

oh, btw, the screenshot needs updating. Also, the requirements for the directory say:

The screenshot must not be bigger than 1200 x 900px
The ratio of width to height needs to be 4:3

Done, now is the right size and is updated with your improvements. The aspect ratio of the design does not match exactly the aspect ratio of the screenshot, and the bottom part is empty, but it does not look that bad.

@MaggieCabrera
Copy link
Collaborator

Done, now is the right size and is updated with your improvements. The aspect ratio of the design does not match exactly the aspect ratio of the screenshot, and the bottom part is empty, but it does not look that bad.

I think we might need to revisit the min height of the grid items. I think I removed them but instead I should have changed them from using px to using rems instead, that would bring it closer to the design. I think it's also ok to take the screenshot directly from figma, and keep working on the pattern, since we might improve it over time and it doesn't make sense to update the screenshot every time we update the code. The design in figma is the north star here

@dballari
Copy link
Author

I think we might need to revisit the min height of the grid items. I think I removed them but instead I should have changed them from using px to using rems instead, that would bring it closer to the design. I think it's also ok to take the screenshot directly from figma, and keep working on the pattern, since we might improve it over time and it doesn't make sense to update the screenshot every time we update the code. The design in figma is the north star here

Yes right heights are not like the design. I just replaced the screenshot by the figma one and now I'll try to keep working on this.

I just had an idea, maybe we could define this layout as a template pattern (like the different options that have been included in the TT5 theme by informing the tag Template Types), since it's still experimental, then we could have several solutions (one with css, maybe one with less css) and we can switch from one to another until we arrive at the best solution, then we could delete the template patterns that we no longer like and finalize the theme.

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.

2 participants