Skip to content

Fix setting controls not scrolling with the rest of the settings menu#465

Merged
AnatoleAM merged 4 commits into
SevenTV:masterfrom
IZEDx:master
Apr 10, 2023
Merged

Fix setting controls not scrolling with the rest of the settings menu#465
AnatoleAM merged 4 commits into
SevenTV:masterfrom
IZEDx:master

Conversation

@IZEDx
Copy link
Copy Markdown
Contributor

@IZEDx IZEDx commented Apr 5, 2023

My twitch has an inline style tag that sets .control { position: absolute; } , which results in the settings menu being unusuable.

I thought it was a compatibility issue as there's no open Issue about that, so I disabled all other twitch extensions and that didn't fix the issue. This is quite a serious issue and since nobody else reported it I guess it's an edge case with my system.

Anyway, this fix is just a single line of css that shouldn't conflict with anything else. I didn't bother to create an open issue for it, I hope that's not a problem.

For reference, this is what it looks like when I scroll down in the settings:

image
image

@AnatoleAM
Copy link
Copy Markdown
Contributor

Isn't this an extension using the same .control class, thereby adding extra styling? If so, it would be better to simply add a prefix to the classes here. This is something we do everywhere else, i.e seventv-settings-...

@IZEDx
Copy link
Copy Markdown
Contributor Author

IZEDx commented Apr 6, 2023

Isn't this an extension using the same .control class, thereby adding extra styling? If so, it would be better to simply add a prefix to the classes here. This is something we do everywhere else, i.e seventv-settings-...

You are right, it'd be better to add a prefix here. I didn't question it, as it's a nested class in scss, but since there's no shadow-dom it'd make sense to future proof all generic class names being used by enforcing prefixes. Question is, does it make sense to make that change in this PR already or leave it as it is, considering this affects many more components and a new Issue has to be created anyway?

Copy link
Copy Markdown
Contributor

@Melonify Melonify left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think prefixing is the way to go here, this is an incompatibility with the Twitch DVR player extension you have enabled.

Setting position to unset is not a good fix here.

@IZEDx
Copy link
Copy Markdown
Contributor Author

IZEDx commented Apr 7, 2023

Yeah, I think prefixing is the way to go here, this is an incompatibility with the Twitch DVR player extension you have enabled.

Setting position to unset is not a good fix here.

Good to know it's an incompatibility issue with Twitch DVR, must've missed it when going through my extensions.

I changed it to use a prefix instead of using position unset. I only added a prefix to the control class however, the other classes should be prefixed in a separate branch for futureproofing. There are quite a lot of classes that have not been prefixed yet scattered around the whole project, which could collide with other extensions in the future.

@IZEDx IZEDx requested a review from Melonify April 7, 2023 23:53
@AnatoleAM AnatoleAM merged commit 4ce84d7 into SevenTV:master Apr 10, 2023
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.

3 participants