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

feat: use k8s.website.tabs to show different installation types #3139

Closed
wants to merge 20 commits into from

Conversation

yuluo-yx
Copy link
Contributor

@yuluo-yx yuluo-yx commented Apr 9, 2024

before:

image

after:

image

show:

eg-docs

Signed-off-by: yuluo-yx <yuluo08290126@gmail.com>
@yuluo-yx yuluo-yx requested a review from a team as a code owner April 9, 2024 03:43
Signed-off-by: yuluo-yx <yuluo08290126@gmail.com>
Copy link

codecov bot commented Apr 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 66.53%. Comparing base (29946b0) to head (a53f6e0).
Report is 52 commits behind head on main.

❗ Current head a53f6e0 differs from pull request most recent head 5395ff6. Consider uploading reports for the commit 5395ff6 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3139      +/-   ##
==========================================
+ Coverage   66.51%   66.53%   +0.02%     
==========================================
  Files         161      161              
  Lines       22673    22693      +20     
==========================================
+ Hits        15080    15098      +18     
- Misses       6720     6722       +2     
  Partials      873      873              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: yuluo-yx <yuluo08290126@gmail.com>
@@ -47,10 +47,16 @@ consideration when debugging.

## Testing the Configuration

{{< tabs name="tabs_test_the_configuration" >}}
Copy link
Contributor

Choose a reason for hiding this comment

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

imo we need to flip this

  1. With External LoadBalancer Support
  2. Without External LoadBalancer Support

cc @eitansuez

Signed-off-by: yuluo-yx <yuluo08290126@gmail.com>
export ENVOY_SERVICE= \
$(kubectl get svc -n envoy-gateway-system \
--selector=gateway.envoyproxy.io/owning-gateway-namespace=default,gateway.envoyproxy.io/owning-gateway-name=eg \
-o jsonpath='{.items[0].metadata.name}')
Copy link
Contributor

Choose a reason for hiding this comment

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

even the curl needs to be part of the tab because its using different ports

@arkodg
Copy link
Contributor

arkodg commented Apr 9, 2024

thanks @yuluo-yx, added some comments !

@yuluo-yx
Copy link
Contributor Author

yuluo-yx commented Apr 9, 2024

/retest

@eitansuez
Copy link
Contributor

if i'm not mistaken this pr will not only apply tabs to the quickstart, but more generally enable the use of tabs in the documentation. once it's merged i'll work on using this feature in additional contexts.

@yuluo-yx
Copy link
Contributor Author

if i'm not mistaken this pr will not only apply tabs to the quickstart, but more generally enable the use of tabs in the documentation. once it's merged i'll work on using this feature in additional contexts.

yep, This is mentioned in #3133.

@arkodg arkodg requested review from Xunzhuo and zirain April 10, 2024 07:06
@eitansuez
Copy link
Contributor

eitansuez commented Apr 10, 2024

i just checked out this PR, the tabs render correctly, but the functionality (switching between tabs) doesn't appear to work: clicking on the currently-selected tab has this undesirable scroll effect, while clicking on the other tab (to switch to it) does nothing. has anyone else tried it and gotten it to work?

@yuluo-yx
Copy link
Contributor Author

i just checked out this PR, the tabs render correctly, but the functionality (switching between tabs) doesn't appear to work: clicking on the currently-selected tab has this undesirable scroll effect, while clicking on the other tab (to switch to it) does nothing. has anyone else tried it and gotten it to work?

my bad, let me check

Signed-off-by: yuluo-yx <yuluo08290126@gmail.com>
@yuluo-yx
Copy link
Contributor Author

yuluo-yx commented Apr 11, 2024

i just checked out this PR, the tabs render correctly, but the functionality (switching between tabs) doesn't appear to work: clicking on the currently-selected tab has this undesirable scroll effect, while clicking on the other tab (to switch to it) does nothing. has anyone else tried it and gotten it to work?

hi, @eitansuez . I fixed , pls review again. tks 🫡

eg-docs

Signed-off-by: yuluo-yx <yuluo08290126@gmail.com>
Signed-off-by: yuluo-yx <yuluo08290126@gmail.com>
@yuluo-yx
Copy link
Contributor Author

/retest

@eitansuez
Copy link
Contributor

Hi @yuluo-yx it works now. thanks for the quick fix.

separately, i agree with @arkodg 's comment about making the "with loadbalancer support" tab the left/default tab. so.. could you please switch the order of the tabs?

otherwise it looks good to me.

@yuluo-yx
Copy link
Contributor Author

Hi @yuluo-yx it works now. thanks for the quick fix.

separately, i agree with @arkodg 's comment about making the "with loadbalancer support" tab the left/default tab. so.. could you please switch the order of the tabs?

otherwise it looks good to me.

Is that right? I'm not sure I understand what you're saying. 🥲🥲

image

Signed-off-by: yuluo-yx <yuluo08290126@gmail.com>
@@ -0,0 +1,41 @@
$(document).ready(function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need this

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

why do we need this

Without these js files, the tab switching action may not be possible, I have streamlined the files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think https://www.docsy.dev/docs/adding-content/shortcodes/#tabbed-panes is enough or what I missed?

I referenced the k8s website as mentioned in #3131

Copy link
Contributor

Choose a reason for hiding this comment

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

can we only add what's required to create tabs e.g. https://learn.netlify.app/en/shortcodes/tabs/
we dont need to copy the k8s theme
I linked k8s as an example to show how tabs were being used in other projects

Copy link
Contributor Author

Choose a reason for hiding this comment

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

can we only add what's required to create tabs e.g. https://learn.netlify.app/en/shortcodes/tabs/ we dont need to copy the k8s theme I linked k8s as an example to show how tabs were being used in other projects

yep, I understand that if I delete these js files locally, the toggle between tabs will not work, so I added these CSS and js files to achieve the toggle effect and did not copy the k8s theme file.

Signed-off-by: yuluo-yx <yuluo08290126@gmail.com>
Signed-off-by: yuluo-yx <yuluo08290126@gmail.com>
@eitansuez
Copy link
Contributor

Is that right? I'm not sure I understand what you're saying. 🥲🥲

yes, that's right. sorry for the late reply.

@yuluo-yx
Copy link
Contributor Author

/retest

@yuluo-yx
Copy link
Contributor Author

Is there another implementation of this feat or are there any other problems with it, if there is an alternative I can close.
@arkodg @eitansuez

arkodg
arkodg previously approved these changes Apr 15, 2024
@arkodg arkodg requested review from shawnh2 and a team April 15, 2024 15:53
@arkodg
Copy link
Contributor

arkodg commented Apr 15, 2024

Hey @yuluo-yx, can you please rebase, we can merge post that

Signed-off-by: YuLuo <yuluo08290126@gmail.com>
@zirain
Copy link
Contributor

zirain commented Apr 16, 2024

this PR is far away than my expected, PTAL #3208

@zirain
Copy link
Contributor

zirain commented Apr 16, 2024

maybe you can rebase your work base on #3208 ?

@yuluo-yx
Copy link
Contributor Author

It is merged, I will close it.

@yuluo-yx yuluo-yx closed this Apr 17, 2024
@zirain
Copy link
Contributor

zirain commented Apr 17, 2024

It is merged, I will close it.

you can rebase with main branch, you did a lot of around better display/html/css, those are very valuable.

@yuluo-yx
Copy link
Contributor Author

It is merged, I will close it.

you can rebase with main branch, you did a lot of around better display/html/css, those are very valuable.

current pr is too confusing, I will resubmit a pr to update the existing file style, thank you for recognizing my work! 🫡

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