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

Manage style rules of AppTile on CallView and StickerPicker on _AppDrawer.pcss #10892

Closed
wants to merge 2 commits into from

Conversation

luixxiul
Copy link
Contributor

@luixxiul luixxiul commented May 13, 2023

For element-hq/element-web#25269

This PR intends to move the rest of the CSS rules which style AppTile's components to _AppDrawer.pcss from the other files.

type: task

AppTileFullWidth AppTileMenuBar
1_1 1_1

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 13, 2023
@luixxiul luixxiul marked this pull request as ready for review May 14, 2023 08:40
@luixxiul luixxiul requested a review from a team as a code owner May 14, 2023 08:40
@luixxiul luixxiul requested review from andybalaam and robintown May 14, 2023 08:40
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 do not think we should move all styles related to AppTile in _AppDrawer.

Thinking about CSS architecture, _AppDrawer should contain generic styling that is inherent to itself.

mx_CallView is a "higher" component that consumes _AppDrawers, and it might need to apply some minor adjustments to it so that it fits in the context.
There is a lot of benefits in collocating this style update with inside the call view CSS file in my opinion.

I am not really seeing the benefit of approaching things the way you suggest. But would be keen to understand the rationale and the maintenance burden you've experienced

@luixxiul
Copy link
Contributor Author

Thinking about CSS architecture, _AppDrawer should contain generic styling that is inherent to itself.

For the context, we surprisingly do not have _AppTile.pcss yet, and everything related to App* have been gathered to the _AppDrawer.pcss (which has not been a proper exercise to begin with). So the recent PRs linked to element-hq/element-web#25269 (comment) intend to concentrate style rules related to AppTile to _AppDrawer.pcss first, so that we can get a clear insight which style rules belong to which components, such as AppWarning, AppPermisison, AppDrawer, AppTile, etc.

@germain-gg
Copy link
Contributor

I understand the intention, and I don't think we should take that direction.
I tried to explain why I think it is not a desirable thing, but I'd like to see it from your point of view too

@luixxiul
Copy link
Contributor Author

Sorry, it was not the answer to your question regarding this PR (and I forgot to tell you that of course we should create CSS files dedicated for each component after getting the clear view. Currently it is even not clear which rules should belong to which files).

Regarding to this PR, I thought it was a policy that managing style rules of a component on the CSS file for the component, regardless of which component requires the style rules, because if we would take highness or lowness into consideration, then I am afraid of it that we would end up having a single mega large file, which we would definitely not want to.

I am not quite sure which one is expected, and if there would be a policy about this matter.

@germain-gg
Copy link
Contributor

It all comes down to a matter of responsibilites. I do not think we will end up with larger CSS files.

If I looks at your changeset

 .mx_CallView .mx_AppTile {
  width: auto;
  height: 100%;
  border: none;
  border-radius: inherit;
  background-color: $call-background;
}

There is two different responsibilities.

border / border-radius are a variant of the mx_AppTile. I would extract that in a mx_AppTile--embedded class. That hosts all the CSS properties an app tile needs to have when being embedded (inside _AppsDrawer.pcss).

And I would have .mx_CallView .mx_AppTile inside _CallView.pcss defining the width, height and background-color.
Because those overrides are ONLY related to the call view. This piece of CSS is here to articulate components in a specific context and should be the lightest override possible.

@luixxiul
Copy link
Contributor Author

border / border-radius are a variant of the mx_AppTile. I would extract that in a mx_AppTile--embedded class.

That is a fantastic idea! 👍 I'm going to check the code again to see how it could be improved.

@luixxiul luixxiul marked this pull request as draft May 15, 2023 08:58
@luixxiul luixxiul closed this Jun 1, 2023
@luixxiul luixxiul deleted the AppTile3 branch June 1, 2023 14:36
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