-
Notifications
You must be signed in to change notification settings - Fork 140
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
Add unequip all gems button to gem summary #4266
base: master
Are you sure you want to change the base?
Conversation
display: flex; | ||
align-items: center; | ||
padding: 0; | ||
color: #ef9eaa; |
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.
Please use a color var for this, or maybe try to copy the btn-reset
styling?
|
||
const headerElement = this.container.headerElement; | ||
if (headerElement) { | ||
const unequipButton = document.createElement('button'); |
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.
Would be nice to just make rename this file to tsx
and use fragments like in Cata
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 was hesitated to do so because making one component into tsx is inconsitent to the other files if no plan to make all of them tsx. But that is my opinion.
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.
You have to start somewhere :D That's how I did it for the Cata repo as well. If you never do it you'll also never change it
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.
Sounds good, I will proceed
Applying the same unequip gems feature from Cata to Wotlk because why not