-
Notifications
You must be signed in to change notification settings - Fork 312
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
Some visual fixes for PWA (no hover, no scroll) #371
Conversation
…when .nav-closed is present
…ionship .relationship\:hidden' and update '.nav-closed .nav-closed\:hidden' to only apply 'hidden' class
* chore(Gemfile.lock): update gem versions * chore(application/_head.html.erb): add conditional logic to load hotwire-livereload tags in development environment * chore(config/environments/development.rb): configure hotwire_livereload.reload_method to :turbo_stream
Hey @jasonpaulso, I pulled down the changes and I'm not sure I'm experiencing them all. The scrolling on login/signup is definitely gone for me. But after I login, I still have to tap the conversations twice to get them to select (the hover state thing) and if I accidentally long tap it still does a flyout. Are they working differently for you? |
Hi @jasonpaulso I should probably just go ahead and merge this in. But I wanted to check and see if you saw my comment: when I tested out the PR I saw that you had fixed the login page scroll but I wasn’t able to experience the other things you mentioned. |
Sorry for my late reply—I took last week off from email, etc. I’ll be having a look at this a bit later this morning, I have a guess at where I belied my PR.On Jun 2, 2024, at 9:52 PM, Keith Schacht ***@***.***> wrote:
Hi @jasonpaulso I should probably just go ahead and merge this in. But I wanted to check and see if you saw my comment: when I tested out the PR I saw that you had fixed the login page scroll but I wasn’t able to experience the other things you mentioned.
—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***>
|
…o nav element * feat(tailwind.config.js): enable hoverOnlyWhenSupported feature in future options
I merged main back into here and resolved the merge conflicts, then I tested your branch out and all the fixes worked for me! I was getting ready to merge in and I noticed that CI system tests are failing on this branch and I'm baffled by it. When I run the tests locally, everything works fine. But in this branch CI runs the system tests and a dozen of them fail. It's so odd! I'm going to try slowly reverting everything one by one in order to isolate what could possibly be causing things to work fine for us locally but not in CI. |
@jasonpaulso I've narrowed the issue down to this change in the tailwind.config:
It causes all of these broken test cases in the CI environment (but not locally): Any idea why? I did a little digging online and don't have a good idea for how to fix. I know we want this change because I assume this is what fixes the hover issue on mobile. I finally came up with a solution which is super hacky: in the CI script it simply uses |
@@ -91,7 +91,10 @@ jobs: | |||
run: bin/rails tailwindcss:build | |||
|
|||
- name: Run system tests | |||
run: bin/rails test test/system | |||
run: | | |||
sed -i 's/hoverOnlyWhenSupported: true/hoverOnlyWhenSupported: false/' config/tailwind.config.js |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For some reason I could never figure out, this setting causes CI tests to fail. This is a really hacky fix, but for now we just revert it right before running tests.
Fixes: