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

Farm Addon & Partner visual adjustments #3700

Merged
merged 3 commits into from
Feb 28, 2025

Conversation

kathyavini
Copy link
Collaborator

@kathyavini kathyavini commented Feb 27, 2025

Description

This is the PR to address visual issues described in #3696 that weren't strictly related to the delete functionality. Addressed:

  1. The focus > hover button layout shift at narrow desktop widths shown here (thank you @SayakaOno for spotting this)
  2. This flash of content / blip on <Partners /> which I'll demostrate here with 4x cpu throttling for clarity -- both the text input and the form buttons are showing momentarily if the connection is active and the RTK query cache is missing (e.g. on first login). Within the PR code, useEffect() address the buttons while the conditional rendering is for the text input flash. Both make use of isLoading which it was my mistake to not include originally!
4x.cpu.throttle.mov

The solution for the flash of buttons also fixes the console warning: Cannot update a component ('AddSensor') while rendering a different component ('Partners').:

Screenshot 2025-02-27 at 10 34 39 AM

Thank you @Duncan-Brain for suggesting a useEffect() yesterday although maybe today you weren't into it 😂 (I think it's a good suggestion though!)

Jira link: none

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

  • Passes test case
  • UI components visually reviewed on desktop view
  • UI components visually reviewed on mobile view
  • Other (please explain)

Checklist:

  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • The precommit and linting ran successfully
  • I have added or updated language tags for text that's part of the UI
  • I have added "MISSING" for all new language tags to languages I don't speak
  • I have added the GNU General Public License to all new files

This is the flashing of 1) the form buttons and 2) the org_id input when the connection is active but the RTK query cache is cleared, e.g. on first login. useEffect() also solves Warning: Cannot update a component ('AddSensor') while rendering a different component ('Partners').
@kathyavini kathyavini self-assigned this Feb 27, 2025
@kathyavini kathyavini requested review from a team as code owners February 27, 2025 18:54
@kathyavini kathyavini requested review from Duncan-Brain and SayakaOno and removed request for a team February 27, 2025 18:54
@kathyavini kathyavini added the bug Something isn't working label Feb 27, 2025
@kathyavini kathyavini marked this pull request as draft February 27, 2025 19:08
@kathyavini kathyavini marked this pull request as ready for review February 27, 2025 19:16
Copy link
Collaborator

@SayakaOno SayakaOno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice fixes, thank you! ❤️

Copy link
Collaborator

@Duncan-Brain Duncan-Brain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think your fixes work well! And useState is good!... I just was trying to figure if using setState in the top level function body was "wrong" but I couldn't find anything.

Also I am maybe confused about how the button styling works. Is it supposed to be active after first click moving forward? Or should the active state clear? The same thing happens with the form buttons in addAnimals.

Screen.Recording.2025-02-28.at.9.40.38.AM.mov

@kathyavini
Copy link
Collaborator Author

kathyavini commented Feb 28, 2025

Also I am maybe confused about how the button styling works. Is it supposed to be active after first click moving forward? Or should the active state clear? The same thing happens with the form buttons in addAnimals.

@Duncan-Brain it's not an active state, it's a focus state. And yup it's correct for that to stay applied until you tab or click elsewhere -- you should see the same behaviour if you tab to or long-click any button in app.

Screen.Recording.2025-02-28.at.9.28.24.AM.mov

We aren't the ones applying those :focus states (that's the browser) so I can only assume it's done correctly, although maybe it's the way it persists through the modal opening and then closing (even if you click on modal buttons) that feels un-intuitive? Or at least it does for me! And most of the time (form buttons as you mentioned an exception) you're triggering a page change away from the button on click so you don't see this.

@Duncan-Brain Duncan-Brain added this pull request to the merge queue Feb 28, 2025
Merged via the queue into integration with commit b6d269a Feb 28, 2025
4 of 5 checks passed
@Duncan-Brain
Copy link
Collaborator

@kathyavini Yeah its the modal and form navigation transitions where I sort of expected the focus state to reset/unfocus/blur whatever haha. Not familiar with native browser behaviour but wondering if we missed something.

Screen.Recording.2025-02-28.at.9.37.10.AM.mov

@kathyavini
Copy link
Collaborator Author

kathyavini commented Feb 28, 2025

@Duncan-Brain for that particular transition in your video maybe it's the fact that the inner text of the button changes but in fact it's the same button (hasn't remounted) so it keeps focus state which feels particularly wrong? Although yeah I guess even if the text didn't change it should feel like a new "next" button 🤔 Let's give it a quick view in tech daily with Anto next week, but for a two step form like that it's a one-line fix with a remounting key on that right-hand button : key={isFinalStep ? 'save-button' : 'next-button'}. (Could do the same with the modal opening in Farm Addon too -- although here I'm less inclined).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants