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

Use classNames function for appTileBodyClass on AppTile.tsx #10939

Merged
merged 2 commits into from
May 19, 2023
Merged

Use classNames function for appTileBodyClass on AppTile.tsx #10939

merged 2 commits into from
May 19, 2023

Conversation

luixxiul
Copy link
Contributor

@luixxiul luixxiul commented May 18, 2023

This PR suggests to use classNames function for appTileBodyClass on AppTile.tsx to improve maintainability, instead of joining class names with white space characters.

Before After
mx_AppTileBody 1
(Mind the white space character at the end of the class name)
1_1
mx_AppTile_loading 1 1
mx_AppTileBody_mini 1
(Mind the white space character at the end of the class name)
1

The class names will be conformed to our naming policy later.

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 18, 2023
@luixxiul luixxiul marked this pull request as ready for review May 18, 2023 12:12
@luixxiul luixxiul requested a review from a team as a code owner May 18, 2023 12:12
Copy link
Contributor

@florianduros florianduros left a comment

Choose a reason for hiding this comment

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

One question

@@ -590,7 +590,11 @@ export default class AppTile extends React.Component<IProps, IState> {
const iframeFeatures =
"microphone; camera; encrypted-media; autoplay; display-capture; clipboard-write; " + "clipboard-read;";

const appTileBodyClass = "mx_AppTileBody" + (this.props.miniMode ? "_mini " : " ");
const appTileBodyClass = classNames({
mx_AppTileBody: !this.props.miniMode,
Copy link
Contributor

Choose a reason for hiding this comment

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

mx_AppTileBody is always is present in the previous version

Copy link
Contributor Author

@luixxiul luixxiul May 19, 2023

Choose a reason for hiding this comment

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

No, because "mx_AppTileBody" + (this.props.miniMode ? "_mini " outputs "mx_AppTileBody_mini " on miniMode.

If miniMode is not enabled, mx_AppTileBody (mind the last white space character) is output.

Here is the how the class name is output on develop.element.io:

1

1

Copy link
Contributor

Choose a reason for hiding this comment

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

You're right

@luixxiul
Copy link
Contributor Author

Thanks for the review!

@luixxiul
Copy link
Contributor Author

@florianduros would you please add to the merge queue?

@florianduros florianduros added this pull request to the merge queue May 19, 2023
Merged via the queue into matrix-org:develop with commit e652177 May 19, 2023
@luixxiul luixxiul deleted the AppTileBody branch May 19, 2023 15:46
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