This repository was archived by the owner on Sep 11, 2024. It is now read-only.
Group common declarations of mx_AppsDrawer--2apps and mx_AppsDrawer--3apps#11108
Closed
luixxiul wants to merge 2 commits into
Closed
Group common declarations of mx_AppsDrawer--2apps and mx_AppsDrawer--3apps#11108luixxiul wants to merge 2 commits into
mx_AppsDrawer--2apps and mx_AppsDrawer--3apps#11108luixxiul wants to merge 2 commits into
Conversation
…3apps Those class names were originally added by 7be5ff0 for a PR which initially implemented a pinned widget, and there does not seem to be a specific reason why "nth-child(3)" was applied for each class name.
100% - 33%*3 = 1%
github-actions
Bot
added
Z-Community-PR
germain-gg
suggested changes
Jun 19, 2023
Contributor
germain-gg
left a comment
germain-gg
left a comment
There was a problem hiding this comment.
I don't understand the need to use 2apps and 3apps at all?
mx_AppsDrawer is a flex container, so why not give mx_AppTiles elements flex: 1; so they take one fraction of the available space?
Setting the width seems extraenous in this scenario? And I definitely do not see a use for the --2apps and --3apps modifiers. An app wide search did not yield any result either
Contributor
Author
Contributor
|
I understand you're not the one who implemented it originally. However you're updating this behaviour and I'm giving you a review based on the diff I have in front of me. If we're going to change the layout code, might as well leverage the |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
For element-hq/element-web#25268
This PR intends to group common declarations of
mx_AppsDrawer--2appsandmx_AppsDrawer--3appsto improve maintainability.Those class names were originally added by 7be5ff0 for a PR #5266 which initially implemented a pinned widget, and there does not seem to be a specific reason why "nth-child(3)" was applied for each class name. It also seems there was not a reason why '33%' was picked instead of
100%/3to count 1%.type: task
Signed-off-by: Suguru Hirahara luixxiul@users.noreply.github.com
Checklist
This change is marked as an internal change (Task), so will not be included in the changelog.