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

Fixes #4318: Made 'Log in to flag review' button shorter and less weird #4710

Merged
merged 2 commits into from Apr 5, 2018

Conversation

Projects
None yet
2 participants
@svitlana-galianova
Contributor

svitlana-galianova commented Apr 2, 2018

#4318 : The button was centered already, so I have fixed width of the pop-up window and made it less 'weird' as was suggested in comments. Please let me know if that is something you were looking for or if you would like to make it shorter.

Before:
screen shot 2018-04-02 at 1 08 24 pm

After:
screen shot 2018-04-02 at 1 07 47 pm

@tofumatt

This is the right idea and looks much better, but I don't think this is the most flexible solution to the issue. Could you try another approach that is this size by default but accommodates extra text?

@@ -14,6 +14,10 @@
.ErrorList {
margin-top: 6px;
}
.TooltipMenu {
width: 220px;

This comment has been minimized.

@tofumatt

tofumatt Apr 3, 2018

Member

I wonder why the width is so much in the first place... this seems good but setting a width on it might mess up other locales. How does this look in German, for instance?

I wonder if width: auto as the default here is messing things up and a width: 100%; would fix it?

This comment has been minimized.

@svitlana-galianova

svitlana-galianova Apr 5, 2018

Contributor

Width 100% on TooltipMenu component is making things even worth:

screen shot 2018-04-05 at 12 56 56 pm

I have changed width to fit-content and it looks good in both German and English now:

screen shot 2018-04-05 at 12 52 54 pm

screen shot 2018-04-05 at 12 51 50 pm

I am not sure if it's ok to use fit-content, it's supported in (according to this website):

Chrome Edge Firefox Internet Explorer Opera Safari
29 16 51 No 44 10.1

This message in English is this weird because it's width is set to be 320px here. In German it looks perfectly fine with width 320px.

This comment has been minimized.

@tofumatt

tofumatt Apr 5, 2018

Member

Those browser numbers are fine with me to support. I think we already use fit-content elsewhere on the site, but either way that looks great. 👍

@tofumatt

🎉 Tested locally and this looks great!

@tofumatt tofumatt merged commit 2e59136 into mozilla:master Apr 5, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
security/snyk No dependency changes
Details

@svitlana-galianova svitlana-galianova deleted the svitlana-galianova:fixing4318 branch Apr 5, 2018

@svitlana-galianova

This comment has been minimized.

Contributor

svitlana-galianova commented Apr 5, 2018

@tofumatt I was looking for new issues and I came across this one #4359 I have checked and this PR fixed #4359 as well. I think that issue should also be closed

@tofumatt

This comment has been minimized.

Member

tofumatt commented Apr 5, 2018

Thanks, I've closed that issue as well. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment