Site banner icon update #300
Conversation
|
@NejcZdovc i think i tried all the things i know how to do to get the Travis build cleared here and it's still failing on me. passes on my local machine though. let me know how i can help. |
|
@rossmoody thanks for submitting! So there's an issue right now that we need to fix with our integration test, so for now one of the travis checks will fail. That said, could you please regenerate and push the snapshot for |
|
@ryanml awesomeness. thanks for the heads up and tip. snapshot updated |
- updated the color and shadow treatment for the icon in site banners to be more visible on light and dark backgrounds Nov -> Dec snapshot update
ryanml
force-pushed
the
site-banner-icon
branch
from
713b9a8
to
da3cc4b
Compare
|
Does this want to be uplifted to anything earlier than v60 @rossmoody @rebron @bradleyrichter ? |
|
Ideally yes.
… On Dec 11, 2018, at 11:55 AM, Pete Miller ***@***.***> wrote:
Does this want to be uplifted to anything earlier than v60 @rossmoody <https://github.com/rossmoody> @rebron <https://github.com/rebron> @bradleyrichter <https://github.com/bradleyrichter> ?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#300 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AM4jqnyVVS3PXCe6BGy9BcBHFOuz1sK1ks5u4A2rgaJpZM4Y8Oys>.
|
|
Get into 59.x and add an uplift request tag if you want us to look at this for 58.x. |
|
@rossmoody uplift request to |
kjozwiak
added
uplift-approved/bc-0.58.x-release
and removed
uplift-request/bc-0.58.x-release
labels
|
@rossmoody @petemill Has this been merged to 0.58.x yet? Also, make sure we get this merged to 59.x. |
|
@rossmoody @rebron @mandar-brave @ryanml @kjozwiak this is in need of an issue in brave-browser which should have the milestone set to 0.58.x so that this item gets sufficiently QA'ed that it's present and working in the release. |
| <svg width='32' height='32' xmlns='http://www.w3.org/2000/svg' xmlnsXlink='http://www.w3.org/1999/xlink'> | ||
| <defs> | ||
| <path d='M28.704 27.28L17.41 15.996 28.704 4.709a1.004 1.004 0 0 0-1.419-1.42L16 14.587 4.715 3.29a1.004 1.004 0 1 0-1.42 1.42l11.296 11.285L3.296 27.281a1 1 0 0 0 0 1.42 1 1 0 0 0 1.419 0L16 17.404 27.285 28.7a1 1 0 0 0 1.42 0 1 1 0 0 0 0-1.42z' id='b' /> | ||
| <filter x='-17.3%' y='-17.3%' width='134.6%' height='134.6%' filterUnits='objectBoundingBox' id='a'> |
There was a problem hiding this comment.
@rossmoody @NejcZdovc @ryanml this may be a bit of an issue in that there are elements in this SVG which have an id attribute. If there are two graphics on the page that both expose these same IDs (which is likely because the ID is 'a' and 'b') then there may be an issue. Probably not an issue with two of the same close-shadow icons on a page, but definitely an issue if two separate graphics which use these same IDs.
The solution is to flatten the SVG so that it does not use DEFs, or to use an external .svg file (which gets an isolated ID scope), or to apply these affects (whatever they are) via CSS.
Incidentally (or related to the above?) why is this called close-shadow? Is it the same as close but with a shadow?
Fixes brave/brave-browser#2614
Updated the color and shadow treatment for the icon in site banners to be more visible on light and dark backgrounds per direction from @bradleyrichter . This feels a little like a bandaid. Bringing the icon in with the shadow as a graphic in this one isolated occurrence seems like bad practice but for now it'll stop people from missing the close button. I imagine this will get revisited soon.