Skip to content
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

#942 preview theme #945

Merged

Conversation

andreicristianpetcu
Copy link
Contributor

@andreicristianpetcu andreicristianpetcu changed the title #942 fix popup preview with transparency #942 preview theme Jul 31, 2020
Copy link
Member

@Rob--W Rob--W left a comment

Choose a reason for hiding this comment

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

Huh nice fix, I didn't even notice it.

What I did notice is that the red dashes are visible when the "Popup Background" is partially transparent. Previously the opaque color of the .browser-popup__caret element would be in front of the outline of the popup, and consequently the triangle would visually appear to be attached to the popup.

Do you have more CSS-magic up your sleeve to fix that? If not then I'll proceed with merging it as is.

@andreicristianpetcu
Copy link
Contributor Author

So you want the dashes to have the same color as the popup? When it's transparent, so should the dashes, right?

@Rob--W
Copy link
Member

Rob--W commented Aug 1, 2020

There is not an issue with the transparency of the red dashes - them being opaque is fine, as they only appear when the user selects a color. I think that my previous comment wasn't clear, so I'll try to be a bit more detailed below.

The (minor) issue is that there are visible dashes at the intersection of the triangle and the rectangle (from the outline around the rectangle). You can also see this in your screenshot, look near the extrusion at the popup. You'll see dashes (which are painted behind the triangle):

@andreicristianpetcu
Copy link
Contributor Author

I looked into the issue with the border and it seems very complicated. I think it can be done with a really complicated clip path but I'm not sure it's worth my time. If it's ok with you, I say merge it as it is and release it. @Rob--W

@Rob--W Rob--W merged commit ae3a15f into mozilla:master Aug 2, 2020
@andreicristianpetcu andreicristianpetcu deleted the fix_popup_and_search_preview branch August 2, 2020 14:21
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.

2 participants