Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix IRC layout, adding a dedicated CSS file for customization and editing rethemendex.sh #21

Draft
wants to merge 22 commits into
base: sc
Choose a base branch
from

Conversation

luixxiul
Copy link

@luixxiul luixxiul commented Mar 1, 2024

This PR intends to achieve these things:

  1. Create a dedicated CSS for customization, making it semi-modular based on @mixin
  2. Align various elements on IRC layout by fixing line-height etc.
  3. Fix visual bugs not specific to IRC layout

--

(1)

When it comes to styling, basically we could just import a custom CSS file after the last existing @import on _components.pcss and put every custom stuff like CSS declarations and variables in it to let them override the inherited values in the way which the concept of CSS expects. This commit also takes advantage of PostCSS to make our style codebase modular by creating files, on which only mixins are defined to be used elsewhere. In order to achieve it, I have edited rethemendex.sh to have it exclude CSS files under _sc from alphabetical sorting and import _sc/_customization.pcss at the end.

The obvious merit of doing so is that it will remove the burden of applying customization directly on the upstream CSS codebase fixing conflicts. Separating customized styles from the upstream codebase should also greatly reduce the manpower to rebase, without being worried about possible regressions, avoding the tech debt of the upstream project which might not be solved. Doing so will also make it easier to fix the regressions on our codebase which the upstream project has not fixed yet. Additionally, it will make contributing easier for those who would like to contribute but are not familiar with the style codebase for SchildiChat, like me. Using @mixin extensively might be opinionated, but this way it should be possible to maintain and develop style rules as modular, as the upstream project has probably hoped somewhat.

Even if properties will be styled with !important by the upstream to cover a new regression, they will be able to be overridden by ones with !important on the CSS files imported later than that. It is better than importing CSS styles and leaving them vulnerable to be accidentally overridden by something imported later than that.

The way in which the upstream project generates concatenated CSS files has been very stable (essentially same since at least 2018. See: https://github.com/matrix-org/matrix-react-sdk/commits/develop/res/css/rethemendex.sh), so we should be able to depend on the current way how it works for a reasonable time.

--

(2)

Based on that, this PR also intends to fix and improve IRC layout, by fixing the wrong Compound implementation introduced by the upstream project.

For reference, I have voluntarily worked on IRC layout to improve it 2022-2023 as a contributor, and most of the style rules I have worked on still remain there and in effect. See:

Nobody at the upstream project does not seem to have fixed the regressions introduced by Compound's wrong implementation, and this PR should fix those issues earlier than Element will eventually.

For IRC layout, this PR intends to fix these regressions inherited from the upstream:

  • Specify the same line-height to any elements on mx_IRClayout by default, letting declarations with more specificity override the value.
  • Align displayName to the right side
  • Make big emoji 2x size of the line-height value
  • Add corner settings to mx_EventTile on IRC layout
  • Apply 14x14 avatar size to EventTile and GELS
  • Align avatar inside ReplyChain (ReplyTile)

--

(3)

This PR also intends to fix bugs not specific to IRC layout:

  • Cancel a duplicated background color value on code snippet area
  • Fix avatar's position on emote in ReplyTile

--

Before After
Message Panel before_1
display names are aligned to left 👎
after_1
display names are aligned to right 👍
Avatar and displayName before_7
Not aligned 👎
after_7
Aligned well 👍
Line highlight on hover before_2
displayName is slightly larger than EventTile 👎
after_2
displayName height is same as EventTile 👍
Alignment before_3
Aligned well 👍
after_3
Aligned well 👍
Avatar on ReplyChain (size and alignment) 2
Not aligned at all (16x16) ❌
1
Aligned (14x14) 👍
Alignment before_3
Aligned well 👍
after_3
Aligned well 👍
Line height on reply tile before_6
Inconsistent ❌
after_6
Consistent 👍
Hidden event via Devtools before_4
Not aligned at all ❌
after_4
Aligned well 👍
Hidden event via Devtools before_4_1
Not aligned ❌
after_4_1
Aligned well 👍
Green ellipsis before_11
Ellipsis is rendered in black 👎
after_11
Ellipsis is rendered in green 👍
With mxId before_11_1
mxID is not aligned with elements around it 👎
after_11_1
mxID is aligned with displayName 👍
after_12_1
On ReplyTile
Profile on ReplyTile before_13
overflow ❌
after_13
Fit well 👍
Big emoji after_5
2x size of line-height

The other bugs which this PR intends to fix:

Before After
Duplicated background color on code snippet area before_8 after_8
Avatar's position on emote before_9 after_9

Before:

before.webm

After:

after.webm

  • I agree to release my changes under this project's license

…IRC layout

This commit does two things:

1. Create a dedicated CSS for customization, making it modular based on mixin
2. Fix line-height on IRC layout

When it comes to styling, basically we could just import a custom CSS file after the last existing `@import` on _components.pcss and put every custom stuff like CSS declarations and variables in it to let them override the inherited values in the way which the concept of CSS expects. This commit also takes advantage of PostCSS to make our style codebase modular by creating files, on which only mixins are defined to be used elsewhere.

The obvious merit of doing so is that it will remove the burden of applying customization directly on the upstream CSS codebase fixing conflicts. Separating customized styles from the upstream codebase should also greatly reduce the manpower to rebase, without being worried about possible regressions. Doing so will also make it easier to fix the regressions on our codebase which the upstream project has not fixed yet.

Even if the properties are annoyingly styled with `!important` by the upstream to cover the regression, they could be overridden by ones with `!important` on the CSS files imported later than that.

The way in which the upstream project generates concatenated CSS files has been very stable (essentially same since at least 2018. See: https://github.com/matrix-org/matrix-react-sdk/commits/develop/res/css/rethemendex.sh), so we should be able to depend on the current way how it works for a reasonable time.
This commit implements the fix modularly.
This commit updates rethemendex.sh, so that any CSS files in "_sc" folder are excluded from being included to alphabetic sorting which causes cascading mess, ensuring `_customization.pcss` is imported at the end of the file.

This also removes warning message from README.md as this update takes care of importing our CSS files.
@luixxiul luixxiul changed the title Fix IRC layout, adding dedicated CSS for customization and editing rethemendex.sh Fix IRC layout, adding a dedicated CSS file for customization and editing rethemendex.sh Mar 1, 2024
@luixxiul
Copy link
Author

luixxiul commented Mar 1, 2024

Following the reasonable suggestion here https://matrix.to/#/!eSTlitQAfqxWWEHMFD:matrix.org/$ezvD9S1tokciSKpyxoSQEv2npu7Y0_cpYAaGR2mJwJQ?via=supercable.onl&via=matrix.org, I will split this PR into two; one for fixing IRC layout and the other for adding a dedicated CSS file for customization and editing rethemendex.sh.

Created the PR: #23

Since displayName on ReplyChain is aligned to the left side on our UI as well, the default way of displaying and hiding mxId with visibility declaration is set to be applied.

This commit also fixes the variable for line height, removing `unset` as it is no longer necessary.
SpiritCroc pushed a commit that referenced this pull request Oct 6, 2024
* Add labels file

Copied from the old matrix-org react-sdk labels (which weren't
synced from github it seems).

* Add label sync workflow

* Remove labels that are defined in element-meta
@luixxiul luixxiul changed the base branch from sc to lite October 22, 2024 10:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant