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

Some visual fixes for PWA (no hover, no scroll) #371

Merged
merged 30 commits into from
Jun 9, 2024

Conversation

jasonpaulso
Copy link
Contributor

Fixes:

  • touch hover states
  • long tap on nav items
  • Login & signup page height
  • adds/re-adds Hotwire reload

Jason Schulz and others added 9 commits May 6, 2024 15:19
…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
- touch hover states
- long tap on nav items
- Login & signup page height
@krschacht krschacht changed the title #298 partial fix Some visual fixes for PWA (no hover, no scroll) May 24, 2024
@krschacht
Copy link
Contributor

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?

@krschacht
Copy link
Contributor

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.

@jasonpaulso
Copy link
Contributor Author

jasonpaulso commented Jun 3, 2024 via email

@krschacht
Copy link
Contributor

krschacht commented Jun 9, 2024

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.

@krschacht
Copy link
Contributor

krschacht commented Jun 9, 2024

@jasonpaulso I've narrowed the issue down to this change in the tailwind.config:

+  future: {
+    hoverOnlyWhenSupported: true,
+  },

It causes all of these broken test cases in the CI environment (but not locally):
https://github.com/AllYourBot/hostedgpt/actions/runs/9438596249/job/25995746220

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 sed to quickly edit the file and flip that flag.

@krschacht krschacht merged commit 96a8b4e into AllYourBot:main Jun 9, 2024
6 checks passed
@@ -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
Copy link
Contributor

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.

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