Skip to content

Conversation

@NeoNyaa
Copy link
Contributor

@NeoNyaa NeoNyaa commented Sep 30, 2022

Fixed the Hide Upgrade Button snippet using the wrong selector

@NeoNyaa NeoNyaa requested a review from a team as a code owner September 30, 2022 22:38
@NeoNyaa NeoNyaa requested review from theRealPadster and removed request for a team September 30, 2022 22:38
@CharlieS1103
Copy link
Member

@Neop0litan If i had to guess, i'd say that this is probably going to be patched soon in the CSS-MAP

@kyrie25
Copy link
Member

kyrie25 commented Sep 30, 2022

@Neop0litan If i had to guess, i'd say that this is probably going to be patched soon in the CSS-MAP

These are 2 component IDs joined together dynamically. It can't be changed via the CSS map.

@CharlieS1103
Copy link
Member

@kyrie25 wdym

@kyrie25
Copy link
Member

kyrie25 commented Oct 1, 2022

Just look around xpui.js yourself. You won't be able to find the absolute class name string anywhere, unlike all of the mapped class name hash.
They do the same thing for other components like TypeElement and they usually use the same ID for multiple parts throughout the UI.

@kyrie25
Copy link
Member

kyrie25 commented Oct 1, 2022

It's better if you can use attributes on your CSS selector for this case tho.

@NeoNyaa
Copy link
Contributor Author

NeoNyaa commented Oct 1, 2022

It's better if you can use attributes on your CSS selector for this case tho.

I opt to avoid using attribute selectors especially when clients can have their language changed. It may work when set to English but, it might not work on another language.

Also note that the previous selector focused the button directly as does mine. The selector may not be as human readable as the last one but, it works until another change is made.

@NeoNyaa NeoNyaa changed the title Hide upgrade button uses new selector Hide upgrade button snippet updated to use new selector Oct 1, 2022
@kyrie25
Copy link
Member

kyrie25 commented Oct 1, 2022

The only attributes that are language-dependent is aria- type, which is meant for accessibility in the first place. href, id, and the alike most likely won't change.

The last one selects the button doesn't mean this also has to, especially when the new element changed and has the same element as sidebar components, which are either li or a.

We aim for maintainability over temporary fixes, because as Spotify gets further updates, the number of selector would stack up also, which would also mean that the user would have to update Marketplace more frequently every time something changes.

@kyrie25
Copy link
Member

kyrie25 commented Oct 2, 2022

Apparently there is a hash class name used in the upgrade button and it will be fixed in spicetify/cli#1966

@kyrie25 kyrie25 closed this Oct 2, 2022
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