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

Fix for multi-column dropdown menues #17

Merged
merged 4 commits into from
Mar 10, 2018

Conversation

ivanhercaz
Copy link
Contributor

Well, I tried to solve the issue #11. Maybe it isn't a good solution, I'm not consider myself the most appropriated to argue it, but it maybe useful as a temporal solution until someone develops something better. This solves the problem with the two-column dropdown, not for the three-column, I tried to solve it but nothing seems fine to me.

In the first commit I made a rule for smaller screens in which the menu have to be shown as one column (check the screenshot I shared in #11). But then, trying another things, I hit upon with another solution to keep the two columns (second commit). At least to me, it doesn't overflow the screen, so it should works.

administrator manual semantic mediawiki org 1

Note: I couldn't test it in my local installation because I couldn't configure successfully the SMW menu system, it's tested with the Firefox's developer tools. If anyone may test it, I would be more relaxed at the time to merge this PR, of course, if @kghbln thinks this PR is useful and solve the problem. If not, I will continue thinking on the solution or in another ways to make the menu system.

@ivanhercaz
Copy link
Contributor Author

I don't know why in the screenshot it isn't appears fine, so I am going to check the code if I can make something to solve, because I imagine that it could be due to the scroller that I see in the responsive view.

@ivanhercaz
Copy link
Contributor Author

ivanhercaz commented Mar 10, 2018

semantic mediawiki extensions semantic mediawiki org

One solution could be to use a smaller font-size (13.4px in the screenshot), but I don't know if it's very useful. Or maybe to keep the items in one column as I showed you, @kghbln, in #11.

P. S. For now, I leave the PR with the multi-columns until you comment what do you think about it.

@kghbln
Copy link
Member

kghbln commented Mar 10, 2018

@ivanhercaz Thanks a lot for looking at this. The code is now live on the wiki but it does not seem to work, at least for me. If you could check with a mobile device. ..

@ivanhercaz
Copy link
Contributor Author

It is very strange, as far as I understand it is unnecessary the use of !important to show the styles in a specific max-widht with a @media query. But, please, when you have time, try adding my last commit, this code:

@media only screen and (max-width: 480px) {
	.dropdown-content {
		left: -100% !important;
		right: auto !important;
		column-gap: 0px !important;
	}
}

It should works without the !important declaration, as it works in another media queries in the stylesheet, for example:

@media only screen and (max-width: 480px) {
	.smwofootercomptb.col-lg-10 {
		nav#p-tb.navbar.navbar-default.p-tb {
			margin-top: 0;
			margin-bottom: 0;
		}
	}
}

But I don't know why this snippet needs it. I will check it, because I read this article, but it isn't the case of a recommendable use for !important, at least in my opinion.

You're welcome Karsten 😄

@ivanhercaz
Copy link
Contributor Author

ivanhercaz commented Mar 10, 2018

@kghbln, remember that in this code there isn't applied the fix for the font-size to show more adapted, as I commented in one of my comment, so if you applied just the code in the PR you will see this a bit overflow. Try adding it to the media query:

font-size: 13.4px;

And if it doesn't work, try using !important.

@ivanhercaz
Copy link
Contributor Author

ivanhercaz commented Mar 10, 2018

Ups, I have checked Awesome Screenshot, the add-on I use to take screenshots, cut the borders so maybe the font-size isn't necessary. First, try without changing the font-size, test and notice me to test it.

P. S. This is what Awesome Screenshot made, check the bottom 😕
semantic mediawiki org

@kghbln
Copy link
Member

kghbln commented Mar 10, 2018

@ivanhercaz Indeed adding !importantdid the trick. Could have figured out this by myself, too. ;) Instead of reducing the font-size which should indeed be avoided; 14px is already too small; I reduced the padding from 6px to 3px. This is now live for you to test.

I also have the impression that we are overly often using !important in spots where is should work without. However, this will be the next step optimizing stuff. I do not want to waste more time here.

About the users manual. We will just have to reduce the number of columns to two and move some less important links out of the menu.

@kghbln
Copy link
Member

kghbln commented Mar 10, 2018

@ivanhercaz I must note that is is absolutely awesome that you looked at this. This really saves me heaps of time.

@ivanhercaz
Copy link
Contributor Author

Indeed adding !important did the trick. Could have figured out this by myself, too. ;) Instead of reducing the font-size which should indeed be avoided; 14px is already too small; I reduced the padding from 6px to 3px. This is now live for you to test.

Now it works fine! Yes, 14px is fine and it wasn't a problem, the problems was my screenshot tool. Nice idea reduce the padding to 3px! I added it to this PR to make the change in the flavour in harmony with the live smw.o site.

screenshot_20180310_135120

I also have the impression that we are overly often using !important in spots where is should work without. However, this will be the next step optimizing stuff. I do not want to waste more time here.

No, no, it is enough for now, at least in my opinion. Tonight I will ask in StackOverflow about this issue with the media query and !importance. And if I achieve the solution, I will make another PR.

About the users manual. We will just have to reduce the number of columns to two and move some less important links out of the menu.

All right! Let me time to code and make some tests and I will open another PR. I think if you are agree with this solution, it can be merged!

@ivanhercaz I must note that is is absolutely awesome that you looked at this. This really saves me heaps of time.

Thank you for your comments @kghbln 😄 I am happy to know that I helped you and save time to you. I'm not a professional developer but with the experience of time and practice I get on sometimes, and sometimes not hehe, with this kind of things.

A huge hug!
Iván

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