-
Notifications
You must be signed in to change notification settings - Fork 42
[C-2763] Fix desktop playlist qa issues #3634
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -59,15 +59,15 @@ | |
} | ||
|
||
.button .icon.left.noText { | ||
margin: 0; | ||
margin: 0 !important; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can't wait for the design updates to avoid things like this in the future There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. right! it's so frustrating |
||
} | ||
|
||
.button .icon.right { | ||
margin-left: var(--unit-3); | ||
} | ||
|
||
.button .icon.right.noText { | ||
margin: 0; | ||
margin: 0 !important; | ||
} | ||
|
||
.button .icon { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,6 +18,7 @@ const messages = { | |
} | ||
|
||
const ImageSelectionButton = ({ | ||
anchorRef: anchorRefProp, | ||
wrapperClassName, | ||
buttonClassName, | ||
hasImage, | ||
|
@@ -32,7 +33,7 @@ const ImageSelectionButton = ({ | |
source, | ||
defaultPopupOpen = false | ||
}) => { | ||
const anchorRef = useRef() | ||
const anchorRefInner = useRef() | ||
const [showModal, setShowModal] = useState(defaultPopupOpen) | ||
|
||
const closeModal = () => { | ||
|
@@ -65,7 +66,7 @@ const ImageSelectionButton = ({ | |
{includePopup ? ( | ||
<> | ||
<Button | ||
ref={anchorRef} | ||
ref={anchorRefProp ? undefined : anchorRefInner} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. curious what this ref stuff is for? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. so remember how the artist popover was weirldy positioned relative to the artwork? the reason was is because the popover was anchored to the button that opens it, not the artwork itself, so i updated the component to accept an anchorRef from a parent (in this case the artwork), and kept the inner one around for compatibility with other usages where we actually want to anchor to the button. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ah interesting. sounds good |
||
className={cn(styles.button, buttonClassName, { | ||
[styles.hide]: showModal | ||
})} | ||
|
@@ -75,7 +76,7 @@ const ImageSelectionButton = ({ | |
onClick={handleClick} | ||
/> | ||
<ImageSelectionPopup | ||
anchorRef={anchorRef} | ||
anchorRef={anchorRefProp ?? anchorRefInner} | ||
className={styles.popup} | ||
error={error} | ||
isVisible={showModal} | ||
|
@@ -113,6 +114,7 @@ const ImageSelectionButton = ({ | |
} | ||
|
||
ImageSelectionButton.propTypes = { | ||
anchorRef: PropTypes.any, | ||
wrapperClassName: PropTypes.string, | ||
buttonClassName: PropTypes.string, | ||
hasImage: PropTypes.bool.isRequired, | ||
|
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.
does anything else use the calm var? change makes sense, but want to make sure that nothing else is affected
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.
yes i went ahead and updated all usages :)