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

Allowed title as props for <details> #3388

Closed
wants to merge 2 commits into from
Closed

Conversation

yathomasi
Copy link
Contributor

From #3383

Additionally, should we use a title prop for the anchor base instead of the first child's text?

Used title as props for <details> tag.

Initially, I only used title as props but there are 2-3 particular cases that use code highlighting in the title.
Screen Shot 2022-03-24 at 19 12 52
So, I also added back the first child's text as a fallback if no title is passed as props so that we still can use markdown as well.
Also, previously in this particular case, the code section seems to be parsed as object object as well
resulting: https://dvc.org/doc/user-guide/external-dependencies#expand-to-see-resulting-object-object-file
This issue should also be fixed with the pr.

But, if markdown syntax is not so important and the title would be enough then the code implementation of the Details component will be very simple and preferable.

@yathomasi yathomasi requested review from jorgeorpinel and a team March 24, 2022 14:35
@shcheklein
Copy link
Member

@yathomasi @jorgeorpinel

Additionally, should we use a title prop for the anchor base instead of the first child's text?

question if this approach breaks some titles and we have to maintain two options, why do we consider it superior and why do ? let's keep the previous one?

also, it's not at all a high priority task to my mind, folks. Unless something is broken now.

@yathomasi
Copy link
Contributor Author

question if this approach breaks some titles and we have to maintain two options, why do we consider it superior and why do? let's keep the previous one?

markdown wouldn't be supported when passed as props but I think it would be okay without it too as it's used in just 2 titles to highlight .dvc.
And, using title as props should make consistent with other markdown custom elements as they also support through props.

Also, previously in this particular case, the code section seems to be parsed as object object as well
resulting: https://dvc.org/doc/user-guide/external-dependencies#expand-to-see-resulting-object-object-file
This issue should also be fixed with the pr.

I think this one needs to be addressed and which we didn't address on pervious pr.

@shcheklein
Copy link
Member

Still, I don't see any clear motivation for this tbh. And still it breaks things in some cases. So, my take - let's postpone / keep it as-is for now.

Copy link
Contributor

@rogermparent rogermparent left a comment

Choose a reason for hiding this comment

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

Not a bad idea, but I think it would be better to separate the bugfixes like the object object thing and the prop-title feature into different PRs, the former is an easy fix we'll want asap while the latter needs some more discussion.

Comment on lines -120 to 122
<details>
<details title="Click and expand to set up the example">

### Click and expand to set up the project
Copy link
Contributor

Choose a reason for hiding this comment

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

The existing header remaining here makes breaks this block
image

<details>

### What is a "local remote" ?
<details title=' What is a "local remote" ?'>
Copy link
Contributor

Choose a reason for hiding this comment

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

While it looks alright since the result is trimmed, we should trim out the excess spaces here.

@julieg18 julieg18 assigned julieg18 and yathomasi and unassigned julieg18 Mar 25, 2022
<details>

### Click and expand to set up the project
<details title="Click and expand to set up the project>
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this PR update all existing <details> blocks? Thanks

@jorgeorpinel

This comment was marked as resolved.

@shcheklein
Copy link
Member

Reminder:

Still, I don't see any clear motivation for this tbh. And still it breaks things in some cases. So, my take - let's postpone / keep it as-is for now.

can we skip addressing the conflict?

please please let's be mindful about the bigger priorities

Comment on lines -173 to +171
### Click and expand to set up example
<details title="Click and expand to set up example">
Copy link
Contributor

@jorgeorpinel jorgeorpinel Mar 29, 2022

Choose a reason for hiding this comment

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

OK just noticed we're postponing this. Just a note that per #3391 these titles should no longer start in "Click for" nor end in "example" (same throughout docs codebase). Thanks

@jorgeorpinel
Copy link
Contributor

Given this is not much of a priority + all the conflicts, should we close it or turn it into an issue? Thanks

@shcheklein shcheklein closed this May 18, 2022
@jorgeorpinel jorgeorpinel deleted the details-title-as-props branch June 17, 2022 03:48
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 this pull request may close these issues.

5 participants