-
Notifications
You must be signed in to change notification settings - Fork 1
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
Small UI feature changes from Igor #245
Comments
Great, thank you for clarifying. That sounds perfect. I'll do a PR for the small changes today. |
Okay sure, I can remove the on hover animation for the children. As when it moves right, it can come off the cursor if you enter from the left. It's possible to make it rise and keep it's original shape too (as in bottom axis stays put, top rises) instead but I'll default to removing it for now as that would take a bit of time regarding alignment with other items in the (flex) row. Thank you for testing it out. Cause Solution |
About the device with the missing menu: I believe because the JS function isMobilePhone which I added computes just once on mounting, if you enter reponsive mode, then the menu won't appear, as it still evaluates to false, but for a real user, they would get the right isMobilePhone value for their device, and not have the menu or Kinder overlapping problem happen. However because A) I can't be totally sure this is what happened for you too? and B) if we see different GUI in development to real users, then that's annoying and problematically not reproducible, I will work out a solution, like solely using tailwind CSS. |
So for me using chrome and the "iPhone 14 Pro Max" the menu disappears, while for e.g. the "iPhone 12 pro" it is displayed. Anyway this isn't a blocking issue to merging this PR, it could well be fine on all real devices and just a bit buggy in the chrome emulated device views, and I agree a solution using only tailwind would likely be more robust. |
Yes I made it use just tailwind. Then it will be robust in the sense that, it will always either by sm, md or lg for a given pixel size or phone devices setting, whatever the tailwind code for CSS classes and media queries uses. So we shouldn't have "gaps" where things are both invisible for both cases of an if statement as such. |
Great then as nothing is blocking, I will merge this and then get on with the user routing today / tomorrow morning. I will come in a bit earlier tomorrow morning and aim to get it functional or PoC for a few pages, by tomorrows stand up. |
Ready for the meeting I didn't want to drill into Button / Flowbite and Tailwind config CSS, as it might have taken too long and meant I had nothing to preview to Igor, but it's possible we can alter the tailwind config in order to continue using etc with pure tailwind (and just "color"=primary/secondary etc). If there is time I will do this, but I prioritize the other issues for now. That would keep things more clear. |
@jmsssc can this issue be closed or is there still something that needs to be done here? |
Good point. The CSS changes (specifically changing classes from loads of Tailwind utils to "btn-primary" etc, and possibly also using tailwidn theming rather than normal css should/could be done) but at this point I'll do that on another issue/time so that you can merge in other branches etc without worrying about needless merge conflicts in relation to that. I'll use #235 as a reference issue for a new branch to do that when there is a quiet time (but for now data import/confirmation with Igor, the logged in test & another project are the main priorities I have) |
I am beginning with these small bits of feedback, then when the unify-navbar, try without login and the cleaned up css class PRs are merged, I will work on the back button functionality (which will affect many pages so I aim to ideally do it one day, get it reviewed and changed without it causing or suffering from any merge conflicts)
The text was updated successfully, but these errors were encountered: