Remove .components-item-group selector in edit-site components by im3dabasia · Pull Request #67384 · WordPress/gutenberg

im3dabasia · 2024-11-28T10:21:14Z

What?

The .components-item-group selector has been replaced with a custom class (.edit-site-sidebar-navigation-screen__item-group) that is scoped specifically to the sidebar navigation screen.

This change does not affect other components outside of the sidebar navigation screen and will prevent unintended side effects if the ItemGroup component is used elsewhere in the future.

Screenshot

github-actions · 2024-11-28T10:21:52Z

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @im3dabasia! In case you missed it, we'd love to have you join us in our Slack community.

If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information.

github-actions · 2024-11-28T10:22:02Z

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: im3dabasia <im3dabasia1@git.wordpress.org>
Co-authored-by: creativecoder <grantmkin@git.wordpress.org>
Co-authored-by: youknowriad <youknowriad@git.wordpress.org>
Co-authored-by: yogeshbhutkar <yogeshbhutkar@git.wordpress.org>
Co-authored-by: SantosGuillamot <santosguillamot@git.wordpress.org>
Co-authored-by: tyxla <tyxla@git.wordpress.org>
Co-authored-by: ntsekouras <ntsekouras@git.wordpress.org>
Co-authored-by: ciampo <mciampini@git.wordpress.org>
Co-authored-by: Mamaduka <mamaduka@git.wordpress.org>
Co-authored-by: manzoorwanijk <manzoorwanijk@git.wordpress.org>
Co-authored-by: mirka <0mirka00@git.wordpress.org>
Co-authored-by: hbhalodia <hbhalodia@git.wordpress.org>
Co-authored-by: sabernhardt <sabernhardt@git.wordpress.org>
Co-authored-by: ellatrix <ellatrix@git.wordpress.org>
Co-authored-by: jameskoster <jameskoster@git.wordpress.org>
Co-authored-by: t-hamano <wildworks@git.wordpress.org>
Co-authored-by: jorgefilipecosta <jorgefilipecosta@git.wordpress.org>
Co-authored-by: imrraaj <imrraaj@git.wordpress.org>
Co-authored-by: ramonjd <ramonopoly@git.wordpress.org>
Co-authored-by: shail-mehta <shailu25@git.wordpress.org>
Co-authored-by: akasunil <sunil25393@git.wordpress.org>
Co-authored-by: talldan <talldanwp@git.wordpress.org>
Co-authored-by: getdave <get_dave@git.wordpress.org>
Co-authored-by: afercia <afercia@git.wordpress.org>
Co-authored-by: gziolo <gziolo@git.wordpress.org>
Co-authored-by: stokesman <presstoke@git.wordpress.org>
Co-authored-by: jsnajdr <jsnajdr@git.wordpress.org>
Co-authored-by: jeryj <jeryj@git.wordpress.org>
Co-authored-by: wwdes <wwdes@git.wordpress.org>
Co-authored-by: creador-dev <creadordev@git.wordpress.org>
Co-authored-by: Mayank-Tripathi32 <mayanktripathi32@git.wordpress.org>
Co-authored-by: prajapatisagar <sagarprajapati@git.wordpress.org>
Co-authored-by: gigitux <gigitux@git.wordpress.org>
Co-authored-by: sgomes <sergiomdgomes@git.wordpress.org>
Co-authored-by: gvgvgvijayan <gvgvgvijayan@git.wordpress.org>

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

t-hamano

Thanks for the PR!

There are no strict rules, but in the Gutenberg project, many components define CSS class names based on the following rules:

{package-name}-{component-name|file-name}

This component is in the edit-site package and the file name is sidebar-navigation-screen-main, so an example of one CSS class name would be edit-site-sidebar-navigation-screen-main.

I don't think we should include the components used inside the component (in this case the ItemGroup component) in the CSS class name.

What do you think about these points?

im3dabasia · 2024-11-28T13:05:50Z

What do you think about these points?

Yes, I saw how the class names are given in the project. I appreciate your example, and I think the name you suggested is appropriate for our context.

I will make the changes right away.

im3dabasia · 2024-11-28T13:12:45Z

I have made the changes as suggested. Please check now, @t-hamano. Let me know if there are any further changes.

t-hamano · 2024-11-29T12:08:21Z

Thanks for the update!

Sorry, I missed an important point. The .edit-site-sidebar-navigation-screen__content .components-item selector is applied to more than just the Design screen. We need to apply horizontal negative margins to these screens as well:

Navigation

gutenberg/packages/edit-site/src/components/sidebar-navigation-screen-navigation-menus/index.js

Line 119 in 52b5429

Pages

gutenberg/packages/edit-site/src/components/sidebar-dataviews/index.js

Line 29 in 52b5429

Templates

gutenberg/packages/edit-site/src/components/sidebar-navigation-screen-templates-browse/content.js

Line 60 in 52b5429

Patterns

For this screen, the edit-site-sidebar-navigation-screen-patterns__group class name is already applied, so we can use it.

Can I use a similar approach to add class names to these components? 🙏

im3dabasia · 2024-12-02T12:07:07Z

Can I use a similar approach to add class names to these components?

Yes @t-hamano , I have done as you have suggested please check now.

Navigation Screen

Pages Screen

Templates Screen

Patterns Screen

Thank you for reviewing my work @t-hamano . Please let me know if any changes are needed in the PR.

t-hamano

Thanks for the update!

Everything works as expected, but it would be nice to improve the code quality a bit more.

Additionally, the latest trunk resolves some issues with mobile layouts. To be on the safe side, it would be a good idea to rebase this PR and retest 🙏

t-hamano · 2024-12-04T09:24:24Z

+.edit-site-sidebar-dataviews {
+	margin-left: -$grid-unit-20;
+	margin-right: -$grid-unit-20;
+}


The element with this CSS class is a parent of the element with the .edit-site-sidebar-dataviews-dataview-item class, so let's move this CSS here.

t-hamano · 2024-12-04T09:25:51Z

+.edit-site-sidebar-navigation-screen-navigation-menus {
+	margin-left: -$grid-unit-20;
+	margin-right: -$grid-unit-20;
+}


Let's move this CSS class here as well.

t-hamano · 2024-12-04T09:27:25Z

 @import "./components/pagination/style.scss";
 @import "./components/global-styles/variations/style.scss";
 @import "./components/sidebar-global-styles-wrapper/style.scss";
+@import "./components/sidebar-navigation-screen-templates-browse/style.scss";


Let's group the CSS related to the navigation screen together. Moving it here seems like a good place to start.

t-hamano · 2024-12-04T09:28:54Z

 	padding: 0 $grid-unit-20;

-	.components-item-group {
+	.edit-site-sidebar-navigation-screen-main {


Can we create a new packages/edit-site/src/components/sidebar-navigation-screen-main/style.scss file and move this style into it?

im3dabasia · 2024-12-04T12:34:48Z

Hey @t-hamano, I’m really sorry for this. I think I made a mistake while doing the rebase. I’ll create a new PR and attach it to this issue. I'm really sorry for any inconvenience caused.

New PR Link : #67575

im3dabasia marked this pull request as ready for review November 28, 2024 10:21

github-actions Bot added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label Nov 28, 2024

t-hamano reviewed Nov 28, 2024

View reviewed changes

t-hamano added [Type] Code Quality Issues or PRs that relate to code quality [Package] Edit Site /packages/edit-site labels Nov 28, 2024

im3dabasia requested a review from t-hamano December 2, 2024 13:37

t-hamano reviewed Dec 4, 2024

View reviewed changes

im3dabasia requested review from adamziel, ajitbohra, artemiomorales, desrosj, draganescu, ellatrix, fabiankaegy, gziolo, juanmaguitar, kevin940726, ndiego, nerrad, ntwb, ryanwelcher, spacedmonkey and tellthemachines as code owners December 4, 2024 12:00

im3dabasia closed this Dec 4, 2024

im3dabasia force-pushed the fix/remove-components-item-group-selector-edit-site-67369 branch from 675da23 to 63999da Compare December 4, 2024 12:15

im3dabasia mentioned this pull request Dec 4, 2024

Remove .components-item-group selector in edit-site components[2] #67575

Merged

Conversation

im3dabasia commented Nov 28, 2024

What?

Screenshot

Uh oh!

github-actions Bot commented Nov 28, 2024

Uh oh!

github-actions Bot commented Nov 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

t-hamano left a comment

Choose a reason for hiding this comment

Uh oh!

im3dabasia commented Nov 28, 2024

Uh oh!

im3dabasia commented Nov 28, 2024

Uh oh!

t-hamano commented Nov 29, 2024

Uh oh!

im3dabasia commented Dec 2, 2024

Uh oh!

t-hamano left a comment

Choose a reason for hiding this comment

Uh oh!

t-hamano Dec 4, 2024

Choose a reason for hiding this comment

Uh oh!

t-hamano Dec 4, 2024

Choose a reason for hiding this comment

Uh oh!

t-hamano Dec 4, 2024

Choose a reason for hiding this comment

Uh oh!

t-hamano Dec 4, 2024

Choose a reason for hiding this comment

Uh oh!

im3dabasia commented Dec 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

github-actions Bot commented Nov 28, 2024 •

edited

Loading

im3dabasia commented Dec 4, 2024 •

edited

Loading