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

feature: resource controls dynamic width #2583

Merged
merged 5 commits into from
Apr 12, 2024

Conversation

anjoseb121
Copy link
Contributor

@anjoseb121 anjoseb121 commented Mar 10, 2024

Description

Resource controls should use the space based on the number of actions it has. Also I've updated the padding style, now it won't show empty space when there's no action.

  • Sets the padding to the first and last child for the resource_controls_component.html.erb component.
  • Removes the w-full style in the resource_controls_component.html.erb
  • Updates the fixed with of the table header to be max-w-24.
  • Removes the px-2 from the table_row_component.html.erb component, the padding is handled by the resource_controls component

Fixes #2404

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works

Screenshots & recording

Screenshot 2024-03-10 at 1 12 43 PM Screenshot 2024-03-10 at 1 13 09 PM

Manual review steps

  1. Load a resource with restricted controls or without any.
  2. Test left and right controls with the config.resource_controls_placement = :left option in the initializer

Manual reviewer: please leave a comment with output from the test if that's the case.

Update width for table_header resource control
Update padding in table_row_component
Copy link

codeclimate bot commented Mar 10, 2024

Code Climate has analyzed commit 529f4fc and detected 0 issues on this pull request.

View more on Code Climate.

@adrianthedev
Copy link
Collaborator

Thanks, @anjoseb121, for this!

I see some issues with left-side placement, and the same is true for the right side. It seems to try to take up more space than it really needs.
We'd like for the controls to only take the space they need.

I think this happens when the page is larger in width than the minimum size table.
Could you please check if that's possible?
Thank you!

CleanShot 2024-03-15 at 17 15 40@2x
CleanShot 2024-03-15 at 17 16 19@2x

@anjoseb121
Copy link
Contributor Author

@adrianthedev thanks for the review! I've fixed it with the last commit

@adrianthedev
Copy link
Collaborator

Hey @anjoseb121 I pushed a few changes to account for some more scenarios.
He're how it looks on my end.
Can you please check on your end and let me know if the results are the desired ones?

CleanShot 2024-03-19 at 11 10 15@2x
CleanShot 2024-03-19 at 11 10 32@2x
CleanShot 2024-03-19 at 11 10 41@2x
CleanShot 2024-03-19 at 11 12 17@2x

CleanShot 2024-03-19 at 11 13 52@2x

@anjoseb121
Copy link
Contributor Author

@adrianthedev Looks great! Thanks for that!

Copy link
Contributor

github-actions bot commented Apr 4, 2024

This PR has been marked as stale because there was no activity for the past 15 days.

@github-actions github-actions bot added the Stale label Apr 4, 2024
@Paul-Bob Paul-Bob removed the Stale label Apr 4, 2024
@adrianthedev adrianthedev merged commit e5302fd into avo-hq:main Apr 12, 2024
19 checks passed
@anjoseb121 anjoseb121 deleted the feature/resource-controls-width branch April 13, 2024 21:02
@xeron
Copy link

xeron commented May 1, 2024

This looks a big ugly when self.record_selector = false and config.resource_controls_placement = :left. Feels like some padding is missing.

Before/After:

Screenshot 2024-05-01 at 12 18 06 PM Screenshot 2024-05-01 at 11 30 33 AM

@xeron
Copy link

xeron commented May 2, 2024

@anjoseb121 @adrianthedev could you take a look please? ^^

@anjoseb121
Copy link
Contributor Author

@anjoseb121 @adrianthedev could you take a look please? ^^

@xeron sure, I can improve it!

@anjoseb121
Copy link
Contributor Author

I just saw @adrianthedev opened a PR for it, thanks for that!

@adrianthedev
Copy link
Collaborator

Yeah. It was a pretty wuick fix

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Resource controls dynamic width
4 participants