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

Improve IRC layout for SchildiChat #269

Closed
wants to merge 1 commit into from
Closed

Conversation

luixxiul
Copy link
Contributor

@luixxiul luixxiul commented Oct 31, 2024

Reason why this PR was submitted for SchildiChat:

Because I like SchildiChat Web, and I no longer want to participate to development of Element Web, while I value Matrix ecosystem as a whole, which is governed by Matrix Foundation.

Until a couple of years I have actively contributed to matrix-react-sdk which was managed by the Foundation. For example, the current implementation of IRC layout is mostly due to my contributions, which are based on previous implementations on _IRClayout.pcss.

For the old repository managed by the foundation, you probably would have expected full transparency of the SDK's development, because it was not under vector-im or element-hq. But it has not really been transparent. The decision making was apparently conducted inside private Matrix rooms, available for Element's employers and employees only, private Figma projects, etc. Element had owned the development of the SDK.

On the repository, some annoying experiences has happened to me. For example, even if you have implemented something for the sake of SDK, that change was soon practically reverted without saying sorry or anything. On another PR, I even got a comment on which a reviewer (who is also the employee of Element) expressed that the person did not welcome my contribution, which must have been nonsense for the person. I revisited the PR to write this, and I remembered how much I have felt annoyed by that shitty attitude toward unpaid contributors like me.

I mean, that was like "what am I doing here, being neither paid nor thanked?"

Due to such kind of annoying experiences, some of which were obviously due to the messy organizational relationship between Matrix Foundation and Element (note: that kind of non-transparency seems to be gradually improving thanks to the Governance Board at the Foundation), I stopped to contribute to matrix-org/matrix-react-sdk. Now this repository was archived and basically merged into element-hq/element-web, which was absolutely right move, and I cannot understand why this has not happened million years ago.

Still, I value Matrix ecosystem as a whole pretty much. While I have decided I would not contribute to Element itself anymore, there are other projects which seem to be interesting, funny, free, and open to foreign contributors than Element is; I'm quite certain that some members will remain hostile against other outside contributors, because they have to earn money to live anyway (which is none of my business, by the way; I also need to earn money to live).

The same kind of resentment I have toward Element can be seen by other people here for example. And, against the comment, the owner encourages them to see SchildiChat as example of community effort.

Indeed, I find this looks to me a fair comment.

So I found it made zero sense for me (or indeed all of us) to "ask" Element to my contribution to review, without me being paid or my contributions honored. Instead, it makes sense to contribute to the project which I like and honor.

To someone who want to merge this change to Element Web: if you remove the credit, I will consult with an organization like Software Freedom Conservancy.


Closes SchildiChat/matrix-react-sdk#21
Retry of #268 (accidentally closed)

This PR is essentially a retry of SchildiChat/matrix-react-sdk#21 and intends to improve the IRC layout for SchildiChat, fixing several issues logged on the upstream repository as well. For detailed explanation, please check the superseded PR of mine.

As I am not quite sure of the workflow for the lite branch yet, please let me know what else to be done.

Upstream SC
Message Panel 5 5_1
Hidden event via Devtools 2 2_1
Alignment 8
Not aligned
8_1
Aligned
Alignment (emote) 3 3_1
Aligned
Spacing 5_1
Wasted space
5
Compact
upstream.mp4
sc.mp4

@luixxiul luixxiul force-pushed the IRC branch 4 times, most recently from 40a3043 to 7bca1e2 Compare October 31, 2024 14:36
Signed-off-by: Suguru Hirahara <[email protected]>
@SpiritCroc
Copy link
Member

SpiritCroc commented Oct 31, 2024

Thank you for the PR!

As I am not quite sure of the workflow for the lite branch yet, please let me know what else to be done.

I'm also not sure yet, it's still quite experimental for how we approach it.
I looked through your PR a bit and I think it may be easier to review if we ask contributors to keep PR-ing to the specific repos the patches apply to, and let us generate the patches on our end instead. Advantage would be that I could expand sections to check context of the changed lines more easily. (We probably want to ensure we squash changes on merge though, so it only ends up in a single patch.)

(Note, for each upstream release, we now have a separate branch in the downstream repos, don't use the default branch for now to PR, until I build myself some automation to update default branches on each upstream release)

If we were to PR patches against this repo, you should use a higher patch number than the existing patches, we want to ensure that those patches related to core SchildiChat changes come before third-party contributions which may be dropped.

On the topic of the patch itself, looks like there's many changes to _EventTile.pcss which do not appear to be specific to IRC layout. I'm fine with accepting PRs that only touch IRC layout, or small changes that affect the other layouts and for which I can clearly see the benefit in my own usage. For anything else I'll forward to su-ex' expertise, which means this would be blocked by a bigger pending tasks of getting the bubble layout going again and blocked by su-ex' time constraints. So I'm wondering if it's necessary to touch all of this? (In a dream world all our downstream CSS changes would go to completely separate files than the upstream ones, overwriting only what is needed, but I haven't really investigated the feasibility of that yet)

@luixxiul
Copy link
Contributor Author

Quick reply to:

On the topic of the patch itself, looks like there's many changes to _EventTile.pcss which do not appear to be specific to IRC layout.

It is indeed hard to see it because of the diff structure, but the patch only changes lines inside mx_EventTile[data-layout="irc"] and mx_GenericEventListSummary[data-layout="irc"], so there is not a change unrelated to the IRC layout.

@luixxiul
Copy link
Contributor Author

I think it may be easier to review if we ask contributors to keep PR-ing to the specific repos the patches apply to.

It might be so for both contributors and reviewers. Let me create one.

@luixxiul
Copy link
Contributor Author

luixxiul commented Oct 31, 2024

Created a PR with the same change here against SchildiChat:sc_v1.11.85: SchildiChat/element-web#4.

Edit: you would have to swap git submodule with your forked repository (example/element-web), which probably is worth mentioning on a documentation for contributors.

@SpiritCroc
Copy link
Member

Edit: you would have to swap git submodule with your forked repository (example/element-web), which probably is worth mentioning on a documentation for contributors.

We already have the submodules pointing to our repos in .gitmodules, so not sure what trouble you experienced. Maybe you cloned the repo before those submodules were updated? Anyway feel free to PR if you have some tips for contributors to put in the README

@luixxiul
Copy link
Contributor Author

Maybe you cloned the repo before those submodules were updated?

Maybe so, but I cannot remember quite well.

Thanks for taking care of SchildiChat/element-web#4 (comment) 😃

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.

2 participants