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

Add markdown tabs #113

Merged
merged 6 commits into from
Sep 30, 2021
Merged

Add markdown tabs #113

merged 6 commits into from
Sep 30, 2021

Conversation

julieg18
Copy link
Contributor

@julieg18 julieg18 commented Sep 29, 2021

  • I went ahead and converted this pull request to draft since I placed test markdown tabs in docs/index.md.

@julieg18 julieg18 self-assigned this Sep 29, 2021
@shcheklein shcheklein temporarily deployed to cml-dev-add-markdown-ta-jejtwu September 29, 2021 15:29 Inactive
@julieg18 julieg18 marked this pull request as draft September 29, 2021 15:29
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.

The implementation logic is a little hard to follow and could possibly be more elegant, but it looks like it works really well such that I'd say it's more than good enough to let in as a first iteration!

src/components/pages/Documentation/Markdown/index.tsx Outdated Show resolved Hide resolved
Copy link
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.

good for me too as a first iteration 🎉 (PS failing link checks were fixed in #110)

@julieg18
Copy link
Contributor Author

The implementation logic is a little hard to follow and could possibly be more elegant, but it looks like it works really well such that I'd say it's more than good enough to let in as a first iteration!

Agreed! I attempted to simplify as much as I could but it definitely still looks complex. Hopefully we can improve it at a later date!

@julieg18 julieg18 temporarily deployed to cml-dev-add-markdown-ta-jejtwu September 29, 2021 20:40 Inactive
@julieg18 julieg18 temporarily deployed to cml-dev-add-markdown-ta-jejtwu September 29, 2021 20:45 Inactive
@shcheklein
Copy link
Member

Could you please clarify - does it render things server side or client side?

@julieg18
Copy link
Contributor Author

Could you please clarify - does it render things server side or client side?

I believe the tabs are rendered server side but input ids are rendered client side.

@shcheklein
Copy link
Member

So, how much of DOM manipulation is happening on the client side? Before we have been trying hard to keep everything client side (to keep JS to minimum, and to keep page as prepare as it could be)

@julieg18
Copy link
Contributor Author

julieg18 commented Sep 29, 2021

So, how much of DOM manipulation is happening on the client side? Before we have been trying hard to keep everything client side (to keep JS to minimum, and to keep page as prepare as it could be)

Hmmmmm, I think I misunderstood your original question. What did you mean exactly by server side/client side? But as for DOM manipulation, I did my best to use the least amount of JS as I could. We use JS to keep track of the input checked state (so we can connect different sets of inputs) and to generate the IDs for the inputs. The rest is HTML and CSS.

@casperdcl casperdcl added design p1-important High priority A: website Area: website labels Sep 29, 2021
@casperdcl casperdcl linked an issue Sep 29, 2021 that may be closed by this pull request
@casperdcl casperdcl added the enhancement New feature or request label Sep 29, 2021
@shcheklein
Copy link
Member

Okay, no worries. I see that we use HTML to define section and markdown supports it as-is (no need to do any transformations client side markup->HTML). I think it should be fine. Sorry for the confustion.

@shcheklein
Copy link
Member

@yalozhkin could you please take a look and suggest how to design this? (not a blocker to merge)

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.

Once the test data is removed, we can merge this first iteration!

@julieg18 julieg18 temporarily deployed to cml-dev-add-markdown-ta-jejtwu September 30, 2021 16:09 Inactive
@julieg18 julieg18 marked this pull request as ready for review September 30, 2021 16:22
@julieg18 julieg18 merged commit a43ff69 into master Sep 30, 2021
@julieg18 julieg18 deleted the add-markdown-tabs branch September 30, 2021 16:23
@casperdcl casperdcl mentioned this pull request Sep 30, 2021
casperdcl added a commit that referenced this pull request Oct 5, 2021
- thanks to #113
- part of #116
@casperdcl casperdcl mentioned this pull request Nov 3, 2021
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A: website Area: website design enhancement New feature or request p1-important High priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Toggle lang button
4 participants