Skip to content

<details> blocks linkable#3329

Merged
yathomasi merged 8 commits into
masterfrom
details-block-linkable
Mar 22, 2022
Merged

<details> blocks linkable#3329
yathomasi merged 8 commits into
masterfrom
details-block-linkable

Conversation

@yathomasi
Copy link
Copy Markdown
Contributor

Made the <details> block linkable and will auto expand when the link is opened through the hashed links.

Some considerations and possible issues:

Fix for #3314

@yathomasi yathomasi self-assigned this Mar 3, 2022
@shcheklein shcheklein temporarily deployed to dvc-org-details-block-l-fpncnf March 3, 2022 10:00 Inactive
@yathomasi yathomasi temporarily deployed to dvc-org-details-block-l-fpncnf March 3, 2022 12:06 Inactive
@shcheklein shcheklein requested a review from casperdcl March 3, 2022 15:13
Copy link
Copy Markdown
Contributor

@julieg18 julieg18 left a comment

Choose a reason for hiding this comment

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

Great job so far!

@casperdcl
Copy link
Copy Markdown
Contributor

Fixes #3314 closes #3299

@casperdcl casperdcl linked an issue Mar 3, 2022 that may be closed by this pull request
Copy link
Copy Markdown
Contributor

@casperdcl casperdcl left a comment

Choose a reason for hiding this comment

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

const id = triggerChildren
.toString()
.replace(/[^a-zA-Z ]/g, '')
.toLowerCase()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

do we need to lowercase?

Suggested change
.toLowerCase()

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Umm, just for the fact that lowercase is better for URL but it is not compulsory. Do we have reason, not to use lowercase?

Copy link
Copy Markdown
Contributor

@casperdcl casperdcl Mar 4, 2022

Choose a reason for hiding this comment

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

Yes capitalisation is nice to keep. We already keep it for our ?tab=XyZ functionality. No strong opinion here though.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

For consistency, wouldn't we want them to be lowercase? All of our other anchor links are lowercase. While tabs do use capitalization, tabs use URL queries, not anchor links.

@yathomasi yathomasi temporarily deployed to dvc-org-details-block-l-fpncnf March 4, 2022 10:46 Inactive
@yathomasi yathomasi temporarily deployed to dvc-org-details-block-l-fpncnf March 4, 2022 11:11 Inactive
@yathomasi yathomasi temporarily deployed to dvc-org-details-block-l-fpncnf March 7, 2022 09:42 Inactive
@yathomasi yathomasi temporarily deployed to dvc-org-details-block-l-fpncnf March 11, 2022 13:33 Inactive
@yathomasi
Copy link
Copy Markdown
Contributor Author

yathomasi commented Mar 11, 2022

This update generates the hash links for titles serially unique on page level(the previous version had uniqueness on whole docs level so there could be hash link starting from numbers even it's occurred 1st time on the page).

This one is simple and for simple titles only it seems to be adopted this way eg: Mermaid sequence diagram. But, the possible problem with this method is updating the page and adding another <details> can alter the numbering causing old links to refer to another place.

Also, I will also be submitting another pr which will have the custom slugger where we can also provide some random code/words through props to make the hash link unique.

@julieg18
Copy link
Copy Markdown
Contributor

julieg18 commented Mar 11, 2022

But, the possible problem with this method is updating the page and adding another <details> can alter the numbering causing old links to refer to another place.

Personally, I don't see this as a huge issue 🤔 Our headings work the same way. Should we avoid making a custom slugger and just use github-slugger?

Also, I will also be submitting another pr which will have the custom slugger where we can also provide some random code/words through props to make the hash link unique.

I feel like hash link creation should be completely automated. Plus, wouldn't that make URLs more lengthy and less readable?

@jorgeorpinel
Copy link
Copy Markdown
Contributor

jorgeorpinel commented Mar 11, 2022

Quick Q. What about

Separately we have the expandable <details> sections which contain details we want to have but hide by default. I guess the same component for general notes could just have a prop to determine if it's auto-hidden.

From #3153 (comment)

?

I.e. will we keep the custom <details> component after admonitions are implemented?

@julieg18
Copy link
Copy Markdown
Contributor

I.e. will we keep the custom <details> component after admonitions are implemented?

I believe so! Collapsible admonitions will probably be created by expanding the <details> component.

@yathomasi
Copy link
Copy Markdown
Contributor Author

yathomasi commented Mar 14, 2022

Personally, I don't see this as a huge issue 🤔 Our headings work the same way. Should we avoid making a custom slugger and just use github-slugger?

I feel like hash link creation should be completely automated. Plus, wouldn't that make URLs more lengthy and less readable?

I also prefer this way as:

  • the level of an impact overall doesn't seem to be huge
  • also the fact that docs maintainer doesn't need to take care of providing the custom code
  • plus we can encourage the docs maintainer to provide a unique title from now on but isn't compulsory.

I.e. will we keep the custom <details> component after admonitions are implemented?

I think the <details> component will still be there, it will still be useful for very long descriptions which are better hidden.
Also, it looks like the plugin mentioned by Roger also supports nested markdown.

Screen Shot 2022-03-14 at 18 59 44

@julieg18
Copy link
Copy Markdown
Contributor

@yathomasi, what's blocking this from being merged? Is it waiting on the decision between using a custom slugger and github-slugger?

@yathomasi
Copy link
Copy Markdown
Contributor Author

@yathomasi, what's blocking this from being merged? Is it waiting on the decision between using a custom slugger and github-slugger?

Yes, @iterative/docs can also help to decide on which could be better. We already kind of have our preference.

@julieg18
Copy link
Copy Markdown
Contributor

julieg18 commented Mar 15, 2022

@iterative/docs, we're discussing adding some props to <details> that help create the anchor link (#3329):

  • code - text that is added to the anchor link
  • position - whether text is added to the beginning or end of the link

Aka

<details code="staring and storing" position="start">

### Expand to see what happens under the hood.

text, text, etc.

</details>

would create a details block with the link being #storing-and-sharing-Expand-to-see-what-happens-under-the-hood. Would these props be useful to you or should we just use the title to create the link?

@yathomasi
Copy link
Copy Markdown
Contributor Author

@julieg18 Thank you for putting this clearly here.

@jorgeorpinel
Copy link
Copy Markdown
Contributor

jorgeorpinel commented Mar 15, 2022

IMO just using the existing title would be ideal e.g. #expand-to-see-what-happens-under-the-hood -- not sure what the utility is for code/position (complicates and lengthens the anchor). Am I missing something?

@jorgeorpinel

This comment was marked as resolved.

@rogermparent
Copy link
Copy Markdown
Contributor

not sure what the utility is for code/position (complicates and lengthens the anchor). Am I missing something?

The initial motivation for code is so that a single docs page can have multiple details blocks with the same summary content, as the resulting ids need to be unique.

@jorgeorpinel
Copy link
Copy Markdown
Contributor

jorgeorpinel commented Mar 15, 2022

Oh I see, good point. We probably shouldn't use headers for <details> titles then, since headers in general are supposed to be unique. Maybe use the title prop instead? And append it with a number in the anchor if needed.

@yathomasi yathomasi marked this pull request as ready for review March 16, 2022 06:55
@yathomasi
Copy link
Copy Markdown
Contributor Author

This PR is still a draft too. Is it ready for review?

Yeah, it's ready for review. To clear the point, we basically have two options to make the id unique:

  1. Using github-slugger(current pr), which will basically add numbers to the slug if it's already there.

examples:

  1. Using custom code based slugger(pr Added custom code based slugger for making <details> blocks linkable #3352 ), for which it will throw error during development time and we need to provide unique code prop in order to make it unique.

The pros and cons of both methods are already discussed(if any queries can be discussed). So, now we need to decide which one should we go with?

@jorgeorpinel
Copy link
Copy Markdown
Contributor

jorgeorpinel commented Mar 16, 2022

I vote for reusing title prop instead of (current behavior) taking the first chile element's text;
and make the anchor unique automatically (append number when needed).
p.s. minor: anchors like the rest of the URL are all lower case right?

@julieg18
Copy link
Copy Markdown
Contributor

julieg18 commented Mar 16, 2022

I vote for reusing title prop instead of (current behavior) taking the first chile element's text;
and make the anchor unique automatically (append number when needed).

So if I'm understanding correctly, we should add a title prop that will become the anchor link URL, and if no title is given, take text from the first child element?

Also, @yathomasi, I don't see any need to use #3352. We can take any props for creating the URL string in the Details component, and have github-slugger handle the rest of the logic. Unless I'm missing something?

@yathomasi
Copy link
Copy Markdown
Contributor Author

I vote for reusing title prop instead of (current behavior) taking the first chile element's text;

So, I assume you mean, provide title through props instead of 1st child. That would just be a different way of providing the title and I also think will be better but it's just the preference for the title.

and make the anchor unique automatically (append number when needed).

For uniqueness, I understand @jorgeorpinel voted for no.1 (current pr) which appends the number if there are two same titles.

p.s. minor: anchors like the rest of the URL are all lower case right?

Yes, it will all be lowercase(just was trying with @casperdcl 's preference).

Also, @yathomasi, I don't see any need to use #3329. We can take any props for creating the URL string in the Details component, and have github-slugger handle the rest of the logic. Unless I'm missing something?

I assume you mean #3352 . Yes, if we go with numbering then we don't need that. But, if we want to throw an error if there are two same titles(for manually setting the unique title) or also if we don't want to have numbering then we would need to go with the custom slugger.

@yathomasi yathomasi temporarily deployed to dvc-org-details-block-l-fpncnf March 17, 2022 12:59 Inactive
@yathomasi
Copy link
Copy Markdown
Contributor Author

yathomasi commented Mar 17, 2022

and make the anchor unique automatically (append number when needed).

@jorgeorpinel This pr exactly does that. So before merging this, we want to be clear. So, here are some points.

  • We are making this <details> block sharable with a link and we don't want those links to change with time.
  • With this approach if we happen to edit the docs and add another <details> block, the numbering can change.
  • if such changes are rare or those changes are acceptable then this method would be best.
  • Otherwise, if the changes are frequent then it would be better to look into another approach(Added custom code based slugger for making <details> blocks linkable #3352)

PS: regarding title on props instead of a first child can be another pr/discussion.
cc: @iterative/docs

Copy link
Copy Markdown
Contributor

@julieg18 julieg18 left a comment

Choose a reason for hiding this comment

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

Great job!

Copy link
Copy Markdown
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.

Looks fine to me!

While the structure of the code shows there shouldn't be any crossover with slugger between pages, just to be sure I looked for two pages that have details blocks with the same summary. Conveniently, start/data-and-model-versioning and start/data-and-model-access both have a block with "Expand to see what's under the hood" (the former having two). I saw no issues, so as far as I can tell everything's clear!

@rogermparent
Copy link
Copy Markdown
Contributor

The commit check is failing on prettier for content/docs/command-reference/repro.md, but that's unrelated to this PR (I think it may have been an earlier issue that's already been solved by @julieg18?)

@yathomasi
Copy link
Copy Markdown
Contributor Author

@julieg18 @rogermparent thanks for the review.

The commit check is failing on prettier for content/docs/command-reference/repro.md, but that's unrelated to this PR (I think it may have been an earlier issue that's already been solved by @julieg18?)

Yes, seems to be the same one. We can create a new issue if it's not. So, merging this one for now.

@yathomasi yathomasi merged commit f7f4b36 into master Mar 22, 2022
@yathomasi yathomasi deleted the details-block-linkable branch March 22, 2022 05:09
@julieg18
Copy link
Copy Markdown
Contributor

The commit check is failing on prettier for content/docs/command-reference/repro.md, but that's unrelated to this PR (I think it may have been an earlier issue that's already been solved by @julieg18?)

Yes! I think this branch might not have been on the latest version of master :)

@jorgeorpinel
Copy link
Copy Markdown
Contributor

jorgeorpinel commented Mar 24, 2022

I'm understanding correctly, we should add a title prop
I assume you mean, provide title through props ... can be another pr/discussion

All HTML tags have the title attribute I think, which is what I was referring to by "reuse". Agree to do that separately because we'll need to also change lots of content simultaneously (remove current header first-childs).

make the anchor unique automatically (append number when needed).

With this approach if we happen to edit the docs and add another <details> block, the numbering can change

Ah wait that's a good point. That may be problematic. Content gets rearranged all the time. A block that was no 2 in the doc may well become no 4 in the future. Well, it's not that common actually but it can definitely happen.

So maybe we should require an id or some other prop that's supposed to be unique somehow after all 😅

@jorgeorpinel
Copy link
Copy Markdown
Contributor

#3383

casperdcl added a commit that referenced this pull request Mar 24, 2022
shcheklein pushed a commit that referenced this pull request Mar 29, 2022
iesahin pushed a commit that referenced this pull request Apr 11, 2022
* details block linkable and expand on current hash

* minor fix

* fixed title props errror added back tabs component

* fix: underscore to dash

* fix: remove react-inlinesvg

* used github slugger above component level

* updated github slugger at the page level

* fix: lowercase slugs
iesahin pushed a commit that referenced this pull request Apr 11, 2022
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.

Make <details> blocks linkable

7 participants