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

Changed bootstrap grid for medium size and above to have text and but… #2055

Conversation

HugsDaniel
Copy link
Contributor

…ton side by side

What? Why?

Closes #1766

I added medium-6 to have both columns side by side for medium size and higher, which makes the button appear on all computers.

What should we test?

Button and text appear side by side on computer.

@sauloperez
Copy link
Contributor

O M G! thank you so much @HugsDaniel ! can you provide at least an screenshot so we can check how it looks like?

@HugsDaniel
Copy link
Contributor Author

No way man

@HugsDaniel
Copy link
Contributor Author

screenshot from 2018-01-24 17-19-16
Jk, here's how it looks

@HugsDaniel
Copy link
Contributor Author

The scrolling problem comes from this in modals.css.scss :

// Prevent body from scrolling when a modal is open
body.modal-open {
  overflow: hidden;
}

and honestly I don't really know how to solve this.
Commenting it out solves the issue and is acceptable for the enterprise registration process because there's no content in the background body, so nothing to scroll. However this applies to all modals, so it may be a problem elsewhere.
I continue the investigation though o/

@sauloperez
Copy link
Contributor

I guess we should then provide a more specific css rule that overrides that one setting overflow: inherit or overflow: auto in that particular case.

@HugsDaniel
Copy link
Contributor Author

Actually the problem is the same for all modals in mobile (I just observed it on signup / login modal as well). It would be worth opening a specific issue don't you think ?

@sauloperez
Copy link
Contributor

sauloperez commented Jan 24, 2018

Agree. If it's a wider issue it will require a longer discussion and an agreement on what to actually do in terms of design. Feel free to open a new one linking to these findings.

@daniellemoorhead
Copy link
Contributor

@sauloperez if you're good to give this a ✅ then it can go to @myriamboure for testing and we can get it merged asap.

@sauloperez
Copy link
Contributor

So @HugsDaniel does this affect any other modal or just this one? I would just fix this one and move on. It is the most problematic one.

@HugsDaniel
Copy link
Contributor Author

HugsDaniel commented Feb 5, 2018

It seems to be affecting any modal. I can fix this one only and move on, no problem. Should I do that ?

@sauloperez
Copy link
Contributor

sauloperez commented Feb 5, 2018

I would, yes @HugsDaniel . It's just a matter of adding small but continuous improvements.

@HugsDaniel
Copy link
Contributor Author

So the login modal now have a max height of 500px on screens from 0 to 600px in width, enabling the user to scroll through the modal on mobile.

@sauloperez
Copy link
Contributor

I took a quick test and I found that we also display a scrollbar from the 3rd last step onwards

popup

I did notice, however, that we keep the location of the scrollbar across steps in the wizard which let's see the button but then, you can't see the wizard's header.

So, can you also remove scrollbar from all these steps @HugsDaniel ?

@HugsDaniel
Copy link
Contributor Author

If I got what you said right, it would be because the modal is adjusting to the size of the window, so I fixed the max-height to 95% on medium screen and up, which should do the trick. Is it what you had in mind ?

@HugsDaniel
Copy link
Contributor Author

Last commit is to apply the scrolling fix from commit #c95a6c2 to all modals.

@sauloperez
Copy link
Contributor

sauloperez commented Feb 9, 2018

I gave another look and there are still couple problems @HugsDaniel . The scroll bar still shows up in the last two steps. Although you can now see part of the button, it's not ideal.

See my testing notes in https://docs.google.com/document/d/18h-Z5iaLN9tvyQrVyt-OqLPnkPXYCrmGraf2dqDwx-g/edit?usp=sharing

I'm wondering if we could avoid the modal and just show a regular page, then the content would likely fit, but if not, it would be easier to see that it falls under the fold. To me the modal in here serves no purpose. What do you think?

@HugsDaniel
Copy link
Contributor Author

I agree the modal is not necessary, although after investigation, showing a regular page instead of a modal brings important modifications to the code and especially to Angular controllers. I'm not yet an Angular expert, so I'm wondering if it's worth it for now.

@daniellemoorhead
Copy link
Contributor

@oeoeaio any thoughts on the above modal vs page discussion?

@sauloperez
Copy link
Contributor

sauloperez commented Feb 16, 2018

If it's hard let's just fix the scrollbars on all the wizard steps @HugsDaniel

@HugsDaniel HugsDaniel force-pushed the 1766_entreprise_registration_scroll_on_first_step branch 2 times, most recently from 5694cbf to 88b77e7 Compare February 19, 2018 12:18
@HugsDaniel
Copy link
Contributor Author

@sauloperez I fixed the max-height to 120% so it can fit properly without showing scrollbars.

@oeoeaio
Copy link
Contributor

oeoeaio commented Feb 19, 2018

We could definitely just put the registration wizard in a regular page instead of using modals. It would not be trivial, but I can't see any major barriers. I am happy to help with this if that is what we decide to do.

@sauloperez
Copy link
Contributor

sauloperez commented Feb 20, 2018

I haven't checked the latest commit from @HugsDaniel but if the content can be seen without the modal's scrollbar, I'd move along. Although my initial idea was exactly the same as yours @oeoeaio, I think that this is a good first step that would solve the issue. What do you think? I just want not to have another long-lasting PR.

@oeoeaio
Copy link
Contributor

oeoeaio commented Feb 20, 2018

I'm happy if you're happy. :)

@mkllnk mkllnk force-pushed the 1766_entreprise_registration_scroll_on_first_step branch from 88b77e7 to f2eee77 Compare March 6, 2018 05:21
@mkllnk
Copy link
Member

mkllnk commented Mar 6, 2018

Staged on https://staging1.openfood.com.au/.

@myriamboure
Copy link
Contributor

Tested on computer looks good: https://www.useloom.com/share/d9bda396ddee44f9a31fe5342e058054
And on phone it's all working good as well, will open another small issue on some UX with logo/banner images box on mobile, but nothing to do with this PR and issue.
SO Ready to go :-)

@sauloperez
Copy link
Contributor

Awesome!

@sauloperez sauloperez merged commit 392d0de into openfoodfoundation:master Mar 6, 2018
@HugsDaniel HugsDaniel deleted the 1766_entreprise_registration_scroll_on_first_step branch June 4, 2018 06:41
@mkllnk mkllnk mentioned this pull request Jul 4, 2018
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.

7 participants