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

Add pattern: Services - Three Columns #114

Merged
merged 16 commits into from
Aug 28, 2024

Conversation

huzaifaalmesbah
Copy link
Contributor

Fixes #80

Copy link

github-actions bot commented Aug 23, 2024

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: huzaifaalmesbah <huzaifaalmesbah@git.wordpress.org>
Co-authored-by: juanfra <juanfra@git.wordpress.org>
Co-authored-by: shail-mehta <shailu25@git.wordpress.org>
Co-authored-by: rejaulalomkhan <rejaulalomkhan@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

Copy link
Member

@juanfra juanfra left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @huzaifaalmesbah!

I left a few comments on the code, to improve some things. I'd like to ask is if we can work on the indentation of the code. And also, if we can include artifacts when we're adding PRs that include changes that are visual (screenshot or screencast will work). Thanks 🙌

patterns/services-three-columns.php Outdated Show resolved Hide resolved
patterns/services-three-columns.php Outdated Show resolved Hide resolved
patterns/services-three-columns.php Outdated Show resolved Hide resolved
patterns/services-three-columns.php Outdated Show resolved Hide resolved
huzaifaalmesbah and others added 2 commits August 23, 2024 20:34
Co-authored-by: Juan Aldasoro <juanfraa@gmail.com>
@huzaifaalmesbah
Copy link
Contributor Author

Thank you for reviewing, @juanfra. I made the changes according to your suggestion.

@rejaulalomkhan
Copy link
Contributor

Thanks for the PR @huzaifaalmesbah
It worked excellently in my tests.

Copy link
Member

@juanfra juanfra left a comment

Choose a reason for hiding this comment

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

Thanks for working on this @huzaifaalmesbah

It looks like we need to do a few adjustments to the layout. And also add the alt to the images. Please check the following screencast

Screen.Recording.2024-08-27.at.15.02.42.mov
patterns/services-three-columns.php Outdated Show resolved Hide resolved
patterns/services-three-columns.php Outdated Show resolved Hide resolved
patterns/services-three-columns.php Outdated Show resolved Hide resolved
patterns/services-three-columns.php Show resolved Hide resolved
@huzaifaalmesbah huzaifaalmesbah deleted the add/services-three-columns branch August 27, 2024 15:58
@huzaifaalmesbah huzaifaalmesbah restored the add/services-three-columns branch August 27, 2024 16:00
@huzaifaalmesbah
Copy link
Contributor Author

Hello @juanfra,
Thank you for your review and suggestions. I’ve implemented the changes based on your feedback. Could you please review my updated PR?

Thanks!

Copy link
Member

@juanfra juanfra left a comment

Choose a reason for hiding this comment

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

Thank you for working on this @huzaifaalmesbah

I still see that the layout needs to respect the width of the designs.

Screen.Recording.2024-08-28.at.09.22.53.mov

On this comment, I showed a way you could potentially fix it. I also see that the code needs indentation.

Can you please work on these things? Thanks in advance.

@huzaifaalmesbah
Copy link
Contributor Author

Huzaifa-20240828184842.mp4
Copy link
Member

@juanfra juanfra left a comment

Choose a reason for hiding this comment

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

Thanks for the changes @huzaifaalmesbah

The layout looks good now. Can you please work on the indentation? Once that's fixed this one should be good to go.

Copy link
Member

@juanfra juanfra left a comment

Choose a reason for hiding this comment

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

Thank you very much @huzaifaalmesbah 🏅

@juanfra juanfra merged commit 9acd9c7 into WordPress:trunk Aug 28, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment