Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Update style rules under mx_AppTileMenuBar_title #10895

Closed
wants to merge 4 commits into from
Closed

Update style rules under mx_AppTileMenuBar_title #10895

wants to merge 4 commits into from

Conversation

luixxiul
Copy link
Contributor

@luixxiul luixxiul commented May 14, 2023

For element-hq/element-web#25269

This is one of the PRs which intend to conform style rules of _AppDrawer.pcss to our naming policy. This PR intends to address the structure of the style rules under mx_AppTileMenuBar_title.

Before After
0_1 1
0_2
0_3
0 1_1

type: task

Checklist

  • Tests written for new code (and old code if feasible)
  • Linter and other CI checks pass
  • Sign-off given on the changes (see CONTRIBUTING.md)

This change is marked as an internal change (Task), so will not be included in the changelog.

@github-actions github-actions bot added Z-Community-PR Issue is solved by a community member's PR T-Task Refactoring, enabling or disabling functionality, other engineering tasks labels May 14, 2023
margin-right: 12px;
}

> :last-child {
Copy link
Contributor Author

@luixxiul luixxiul May 14, 2023

Choose a reason for hiding this comment

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

.mx_AppTileMenuBar_title > :last-child was this span element below. It is not clear why it was specified so (instead of .mx_AppTileMenuBar_title > span), while there has not been another element directly under mx_AppTileMenuBar_title.

@luixxiul luixxiul marked this pull request as ready for review May 14, 2023 08:47
@luixxiul luixxiul requested a review from a team as a code owner May 14, 2023 08:47
Copy link
Contributor

@germain-gg germain-gg left a comment

Choose a reason for hiding this comment

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

I'm struggling to see the optimisation here?
Why is it a bad thing to have an inline element?

@luixxiul
Copy link
Contributor Author

I do not have a strong opinion of inlining styles, so I am going to revert that change.

@germain-gg
Copy link
Contributor

@luixxiul I think the question spans for the entire PR ?
You mention confirming to the naming policy, but I only see you created some new CSS classes and not changed old ones? Same goes with the inline style... I don't think the structure is perfect, but it seems to be holding its own weight?

@luixxiul
Copy link
Contributor Author

luixxiul commented May 15, 2023

You mention confirming to the naming policy, but I only see you created some new CSS classes and not changed old ones?

I am not quite sure if I understood you well, but I intentionally am trying to split tasks for the sake of reviewers who prefer small PRs. If we address several tasks on this PR, we would also rename mx_AppTileMenuBar into mx_AppTile_menuBar, creating _AppTile.pcss, moving the style rules to the file, but I would like to address them with other PRs.

@luixxiul luixxiul changed the title Optimize mx_AppTileMenuBar_title Update style rules under mx_AppTileMenuBar_title May 15, 2023
@germain-gg
Copy link
Contributor

I'm going by the description of the pull request here, and you mention that the changes are to conform the classes names with our styleguide.
But I do not see any changes related to that besides mx_AppTileMenuBar_title_widgetAvatar, and I do not think it was violating the style guide in the first place.

The class is scoped within mx_AppTileMenuBar, so that should not be a concern.


But I would also like to address the small pull request comment.
I believe it is important to a small pull request. The ideal is to make the smallest pull request possible for a coherent unit of work.

@luixxiul
Copy link
Contributor Author

The class is scoped within mx_AppTileMenuBar, so that should not be a concern.

Yes, I would agree.

Despite the difficulty of understanding how > :last-child works, the PR seems to have become irrelevant.

@luixxiul luixxiul closed this Jun 1, 2023
@luixxiul luixxiul deleted the AppTileMenuBar branch June 1, 2023 14:45
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
T-Task Refactoring, enabling or disabling functionality, other engineering tasks Z-Community-PR Issue is solved by a community member's PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants