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: add visible option to panels #2989

Merged
merged 4 commits into from
Jul 18, 2024

Conversation

icaroryan
Copy link
Contributor

@icaroryan icaroryan commented Jul 15, 2024

Description

This PR adds a visible option to Panels, which should hide all the contained fields as well as the panel.

This new option shouldn't affect main_panel only panel

Fixes #https://github.com/avo-hq/avo/issues/2971

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

Screen.Recording.2024-07-15.at.10.15.21.AM.mov

Manual review steps

  1. Add visible: -> { false } to a panel
  2. Make sure it hides all the panel header and all fields contained
  3. Move set it back to true and they should reappear
  4. Ensure it doesn't work when changing the visibility of main_panel
  5. Ensure it doesn't hide anything when visible isn't set

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

Copy link

codeclimate bot commented Jul 15, 2024

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

View more on Code Climate.

@icaroryan
Copy link
Contributor Author

Ayy, my first collaboration!
I was wondering where I'd add automated tests for this feature. I couldn't find the proper file to write them

@Paul-Bob
Copy link
Contributor

Hello @icaroryan you made it!

Thanks for the contribution, it's looking good.

Most Avo test files are under the spec/features/avo/** and spec/system/avo/** paths.

We have this test that already checks the visibility of tabs, panels, and the sidebar. The purpose of this test is to ensure that none of these elements are rendered if their respective fields are hidden. You can add a test case for panel visibility block inside it.

@icaroryan
Copy link
Contributor Author

icaroryan commented Jul 17, 2024

Thank you @Paul-Bob! Just pushed the new tests. I tried to follow the same structure of the existing ones

@icaroryan icaroryan force-pushed the feature/add-visible-option-to-panels branch from 569c94f to c1079ec Compare July 17, 2024 19:30
@icaroryan
Copy link
Contributor Author

With that being said, I think it’s worth having the same option for tab. If you give me the go-ahead, I can push a PR for it. It would be basically the same thing we did with panels.

@icaroryan icaroryan force-pushed the feature/add-visible-option-to-panels branch from c1079ec to 74e3b86 Compare July 17, 2024 19:41
@Paul-Bob
Copy link
Contributor

Thank you @Paul-Bob! Just pushed the new tests. I tried to follow the same structure of the existing ones

Looking great, Thanks!

DOCS PR (just to remain linked): avo-hq/docs.avohq.io#248

With that being said, I think it’s worth having the same option for tab. If you give me the go-ahead, I can push a PR for it. It would be basically the same thing we did with panels.

I would be happy to merge it, thanks for the initiative, we appreciate it!

@Paul-Bob Paul-Bob merged commit 5ec14fa into avo-hq:main Jul 18, 2024
21 of 22 checks passed
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.

Add the visible option to Panels
2 participants