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 tab #3017

Merged
merged 1 commit into from
Jul 19, 2024

Conversation

icaroryan
Copy link
Contributor

@icaroryan icaroryan commented Jul 18, 2024

Description

This PR adds a visible option to Tab, which should hide all the contained fields as well as the tab itself.

This new option shouldn't affect tabs only tab

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-17.at.3.55.54.PM.mov

Manual review steps

  1. Add visible: -> { false } to a tab
  2. Make sure it hides all the tab header and all fields contained
  3. Move set it back to true and they should reappear
  4. 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 18, 2024

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

View more on Code Climate.

Copy link
Contributor

@Paul-Bob Paul-Bob left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution @icaroryan!

Could you also make a PR with the docs, similar to what you did with the panel visible option?

BTW, would it be difficult to add the visible options to the tabs DSL too?

@Paul-Bob Paul-Bob merged commit 4b4c367 into avo-hq:main Jul 19, 2024
23 checks passed
@icaroryan
Copy link
Contributor Author

icaroryan commented Jul 19, 2024

Hi @Paul-Bob! I'll add documentation for it as soon as I can.

As for the tabs, DSL, the biggest struggle is just how args are being passed into the builder.
add_item Avo::Resources::Items::TabGroup::Builder.parse_block(parent: @parent, id: id, name: name, **kwargs, &block) (holder.rb)
When you pass it like this, you look for the argument visible in the constructor, which you cant find

tab_group.rb

def initialize(name:, id:, parent:, style: nil)
   @group = Avo::Resources::Items::TabGroup.new(name: name, id: id, style: style)
   @items_holder = Avo::Resources::Items::Holder.new(parent: parent, from: self)
end

So I think it'd be just a matter of adding a visible argument, or restructure how **kwargs is being used, and use **args instead, like the others

Is there a reason why we're using **kwargs?

@icaroryan
Copy link
Contributor Author

icaroryan commented Jul 19, 2024

I think I was rubber-ducking in the message above, I'm sorry lol, the solution was just simply accepting the **args in the argument list for the tabs constructor

#3031

@Paul-Bob
Copy link
Contributor

Thank you! I added this task on docs for it avo-hq/docs.avohq.io#250

Is there a reason why we're using **kwargs?

**kwargs is actually the same as **args and would be the same as **the_name_doesnt_matter_that_much

kwargs stands for keyword arguments and args stands for arguments, IMO kwargs is a better fit since the double splat operator (**) is intended for keyword arguments but the name is not that important, on more recent ruby versions you can even use anonymous arguments .

If you inspect it you'll notice that's still a hash.

If you are up to this challenge (add the visible option for tabs) I would be happy to guide you.

@Paul-Bob
Copy link
Contributor

I think I was rubber-ducking in the message above, I'm sorry lol, the solution was just simply accepting the **args in the argument list for the tabs constructor

https://github.com/avo-hq/avo/pull/3031/files

No need to apologize! It looks like this was not such a challenge after all... :D

Thank you!

@icaroryan
Copy link
Contributor Author

@Paul-Bob Yea, after looking a bit closer at the code I noticed it was just a naming difference. Thank you for the explanation <3

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.

2 participants