-
-
Notifications
You must be signed in to change notification settings - Fork 982
fix: Configurable essentials max fix conflict of PR 8441 #9189
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
base: dev
Are you sure you want to change the base?
Conversation
Signed-off-by: Blake Gearin <hello@blakeg.me>
Signed-off-by: Blake Gearin <hello@blakeg.me>
Signed-off-by: Blake Gearin <hello@blakeg.me>
Signed-off-by: Blake Gearin <hello@blakeg.me>
Should we close #8441 ? |
I think so, I think the author hasn't seen it yet and it will probably take a while to see it, I decided to go ahead a little to see if you would accept the PR, but the author of this PR that I marked is not me, we are different people, just to be clear. |
Before merging this PR, I had one thought, what do you think about us establishing certain confirmation messages (pop-ups) to be opened, when certain amounts of essential tabs are changed? so for example, 4 essential tabs open => generate a warning that if you have 8gb of ram or less this may affect performance, 8 essential tabs open => generate a warning that if you have 16gb of ram or less this may affect browser performance and consume a lot of RAM (and this could considerably reduce most of the issues that people end up opening here on github, because if they read it, they will have an idea of what is happening, because the warning was displayed), remembering that these warnings about the number of tabs and amount of ram are merely hypothetical, no calculation was made about this, this would just be a way to avoid many issues being opened for a problem that would be easily solved just by removing some essential tabs, anyway, please give your opinion on this and what you think about it, if this has already been discussed or not in the previous PR, I did see something like this being discussed, but not together with this new feature. |
src/zen/workspaces/ZenWorkspaces.mjs
Outdated
name = 'Space', | ||
icon = undefined, | ||
dontChange = false, | ||
name, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't recognize the changes from here down
The name value probably needs to have an initial value, so I put the default value back, but the icon, from what I understand from Zen and also from JavaScript, does not need to have an undefined value, since by default, without receiving any value, it is already undefined. Signed-off-by: Mirai <mirai.dev@outlook.com>
…ding essential container updates and improved handling of extra tabs, b=no-bug, c=workspaces
…iDevv/desktop into configurable-essentials-max
I realized that I had removed some things that were important, I put them back looking at the code that was there before, if anyone has any more changes that need to be made or thinks that something is different, you can let me know, please, have a good day. |
This should be without a second pref for height, it should be dynamic |
…ZenUIManager and CSS, simplifying the vertical tabs implementation
…iDevv/desktop into configurable-essentials-max
Is that what you meant? From what I understood, it was just to remove the logic that was determining the height, if not, if you can point to the line/file or explain more comprehensively, if it's not too much trouble. |
…tab management (it had another identical method above).
That's exactly what I mean, why is ZenWorkspaces.mjs a new created file though? |
I didn't understand either, I just remember that I accidentally removed some lines and then put them back, the last time I compared the two files they were the same, I won't be sure just by saying, but I think it was a github bug, because I didn't even delete the file........... |
Just wanted to call out this has been awaiting approval for over 60 days now (factoring in both PRs) |
resolve conflict of PR: #8441