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

Consolidate how we set default tag and allowed tags for accurate documentation #491

Closed
1 of 5 tasks
khiga8 opened this issue Apr 27, 2021 · 7 comments · Fixed by #673
Closed
1 of 5 tasks

Consolidate how we set default tag and allowed tags for accurate documentation #491

khiga8 opened this issue Apr 27, 2021 · 7 comments · Fixed by #673
Assignees

Comments

@khiga8
Copy link
Contributor

khiga8 commented Apr 27, 2021

In System arguments, we have:
All Primer ViewComponents accept a standard set of options called system arguments. This includes a tag option but it isn't necessarily true that a consumer can update tags. In reality, some of our components don't allow tags to be change since we set internally:

system_arguments[:tag] = :input

Other times, we have a default and allow tag to be changed based on user input:

system_arguments[:tag] ||= :input

OR

 def initialize(
      tag: DEFAULT_TAG,

And other times, we have a default tag which can be changed to limited tag options.

@system_arguments[:tag] = fetch_or_fallback(TAG_OPTIONS, tag, DEFAULT_TAG)

The documentation for components don't always surface this information making it unclear what the default tag is, what tags are allowed, etc.

Our Rake populates the default tag section by looking the component initializer and checking if there's a tag keyword:

 def initialize(
      tag: DEFAULT_TAG,

However, we don't always set default tag like this so that section in the doc ends up beingN/A for most components. This is a problem because it is ambiguous without looking at the component source code what tags are allowed, what the default tag is, etc.

TODOS:

  • Remove tag from system arguments docs since it's misleading
  • Create tag allow list (currently known as TAG_OPTIONS) for the components and slots that do allow restricted set of tags. These allowed tags should be surfaced in the docs in Arguments table. Looks like we do this for a few components already like Button.
  • For components that can render any tag, declare a tag parameter with a default.
  • For components that have fixed tags that can't be changed, no need to document in Arguments though we may mention what the tag is if it's important.
  • Maybe.... update Contributing docs?
@khiga8 khiga8 self-assigned this Apr 27, 2021
@joelhawksley
Copy link
Contributor

@khiga8 off the top of my head, I think we should be explicitly allow listing tags for every component.

@khiga8
Copy link
Contributor Author

khiga8 commented Apr 28, 2021

from 4/28 pairing:

  • Remove tag from system arguments docs since it's misleading
  • Create tag allow list (currently known as TAG_OPTIONS) for the components and slots that do allow restricted set of tags. These allowed tags should be surfaced in the docs in Arguments table. Looks like we do this for a few components already like Button.
  • For components that can render any tag, declare a tag parameter with a default. Also surface this information in Arguments. I don't think there will be too many components that accept any tag??
  • For components that have fixed tags that can't be changed, no need to document in Arguments though we may mention what the tag is if it's important.

@khiga8
Copy link
Contributor Author

khiga8 commented Apr 29, 2021

TO REVIEW - components / slots that allow flexible tag

  • avatar_stack_component
  • avatar_stack_component body_arguments
  • blankslate_component
  • button_group
  • details_component :body slot
  • hidden_text_expander
  • label_component
  • markdown_component
  • menu_component :heading slot
  • popover_component
  • popover_component :heading slot
  • subhead_component :heading slot
  • tab_nav_component
  • text_component
  • tooltip_component
  • truncate
  • underline_nav_component :actions slot
  • navigation/tab_component :panel slot

@khiga8
Copy link
Contributor Author

khiga8 commented Jun 21, 2021

6/21/21 Component-driven UI meeting

  • Leave out Tooltip because the Tooltip API itself is questionable. Debate about whether it should be a component.

@joelhawksley
Copy link
Contributor

@khiga8 I still think we should have an allow list of some sort for Tooltip. Do you think there's a minimal approach we could take to land something, even if it's not final?

@khiga8
Copy link
Contributor Author

khiga8 commented Jun 30, 2021

I chatted with @manuelpuyol in my pairing session last week about what the allowed tags for Tooltip should be. The conversation shifted and both of us reached the conclusion that Tooltip is odd for a number of reasons and we should direct our efforts to either deprecating it or converting it to system_arguments. Tooltip will be out of scope of this issue, but I've opened an issue #663 to follow up.

@joelhawksley
Copy link
Contributor

@khiga8 sounds good. Want to close this out?

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 a pull request may close this issue.

2 participants