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

Deprecate mx_RightPanel_headerButton class #10821

Merged
merged 6 commits into from
May 16, 2023
Merged

Deprecate mx_RightPanel_headerButton class #10821

merged 6 commits into from
May 16, 2023

Conversation

luixxiul
Copy link
Contributor

@luixxiul luixxiul commented May 8, 2023

For #10495

This PR intends to deprecate mx_RightPanel_headerButton class in favor of mx_RoomHeader_button to update the header buttons of RoomHeaderButtons by conforming them to our naming policy.

Moving the styles to _RoomHeader.pcss or _RoomHeaderButtons.pcss should be addressed by another PR.

type: task

Signed-off-by: Suguru Hirahara [email protected]

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 8, 2023
@luixxiul luixxiul marked this pull request as ready for review May 12, 2023 07:17
@luixxiul luixxiul requested a review from a team as a code owner May 12, 2023 07:17
@luixxiul luixxiul marked this pull request as draft May 13, 2023 02:29
- %s/mx_RoomHeader_button_highlight/mx_RoomHeader_button--highlight/g
- %s/mx_RoomHeader_button_unread/mx_RoomHeader_button--unread/g
@luixxiul
Copy link
Contributor Author

The Jest snapshot update by #10807 was applied.

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 can get behind the renaming of the CSS class, makes sense 👍
Would you be able to move the CSS in the file you suggested now? That would be quite relevant as part of this changeset. And I don't see the need for a different pull request.

@luixxiul
Copy link
Contributor Author

luixxiul commented May 15, 2023

And I don't see the need for a different pull request.

Some reviewers prefer to review a PR which is dedicated to a single task, rather than including multiple ones.

Because it has been intended for the sake of a reviewer, it is no problem for myself to create a new CSS file on this PR.

Please wait for a moment…

@germain-gg
Copy link
Contributor

I very much agree with the fact that a pull request should contain one change. I don't think renaming and moving them to a new file should be considered separate in this scenario. It is part of one unit of work.

@luixxiul
Copy link
Contributor Author

Indeed. I just thought the "single" change in terms of the number 😉

- Move Sass variables to the place where they are used
@luixxiul

This comment was marked as resolved.

@luixxiul luixxiul marked this pull request as draft May 15, 2023 08:27
@luixxiul

This comment was marked as resolved.

@luixxiul
Copy link
Contributor Author

The conflict was fixed.

@luixxiul luixxiul requested a review from germain-gg May 16, 2023 11:21
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.

Thank you for making the changes! Looks great

@germain-gg germain-gg enabled auto-merge May 16, 2023 15:22
@luixxiul
Copy link
Contributor Author

Thanks for the review!

@germain-gg germain-gg added this pull request to the merge queue May 16, 2023
Merged via the queue into matrix-org:develop with commit e01d479 May 16, 2023
@luixxiul luixxiul deleted the HeaderButtons4 branch May 16, 2023 19:19
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