Skip to content

Conversation

Ekwuno
Copy link
Contributor

@Ekwuno Ekwuno commented Jun 20, 2022

Since Workers introduced module syntax, we need a way to show the difference between service worker syntax and module syntax in our docs.

This PR, adds a shortcode that can be used within our current docs setup as we would an Aside for example.

You can test it at https://add-tab-switcher-shortcode-c.cloudflare-docs-7ou.pages.dev/pages/tutorials/forms/

Todo:

  • Fix responsiveness

@github-actions github-actions bot requested a review from deadlypants1973 June 20, 2022 01:24
@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Jun 20, 2022

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 39eed6f
Status: ✅  Deploy successful!
Preview URL: https://8ac54c53.cloudflare-docs-7ou.pages.dev
Branch Preview URL: https://add-tab-switcher-shortcode-c.cloudflare-docs-7ou.pages.dev

View logs

@Ekwuno Ekwuno removed the request for review from deadlypants1973 June 21, 2022 06:14
@kodster28
Copy link
Collaborator

From a bit of testing, looks like only the final example (TS (SW)) allows you to use the scroll down functionality to see the entire snippet.

@Ekwuno
Copy link
Contributor Author

Ekwuno commented Jun 29, 2022

From a bit of testing, looks like only the final example (TS (SW)) allows you to use the scroll down functionality to see the entire snippet.

Yea! Ignore this for now. It's a draft because I have to fix that bug. I have actually, but on a different branch.

@kodster28
Copy link
Collaborator

One question.. can't quite grok it just from looking at the code.

Is the tab component generic enough to be used throughout the docs site? i.e., like for helping us group examples like this on this page into a two-tabbed Dashboard / API structure?

@Ekwuno
Copy link
Contributor Author

Ekwuno commented Jun 29, 2022

examples like this

Yes, it is! You will write the Shortcode just as you would an Aside

@Ekwuno Ekwuno marked this pull request as ready for review June 30, 2022 10:44
@Ekwuno
Copy link
Contributor Author

Ekwuno commented Jun 30, 2022

@kodster28 you can test it now

Co-authored-by: Gift Egwuenu <[email protected]>
@jgentes
Copy link
Contributor

jgentes commented Jun 30, 2022

Looks like there's an odd hover effect that goes away if width: 25% is removed from .tab-label-wrapper:
image

Copy link
Contributor

@kristianfreeman kristianfreeman left a comment

Choose a reason for hiding this comment

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

quick bug I found. also, is there any sort of active tab CSS styling?

assets/events.ts Outdated
const tabs = wrappers[i].querySelectorAll(".tab");

// Set the first tab in a group to display
((tabs[0] as HTMLElement) || null).style.display = "block";
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like it fails for me (Safari) about 50% of the time when I refresh the page, so that no active tab shows up.

Screen Shot 2022-06-30 at 3 16 27 PM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, now the browser wars. Let me look into how to fix that for safari. I tested on Mozilla and chrome. There is an active tab styling.

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 am not able to recreate this. I launched safari for the first time ever! and It works as expected.

Screenshot 2022-06-30 at 15 28 58

Could this be some caching problem? I will look into it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@codewithkristian I have put up some changes. Can you check if this behaviour presets?

@Ekwuno Ekwuno requested a review from kristianfreeman July 15, 2022 14:25
@gitguardian
Copy link

gitguardian bot commented Jul 16, 2022

️✅ There are no secrets present in this pull request anymore.

If these secrets were true positive and are still valid, we highly recommend you to revoke them.
Once a secret has been leaked into a git repository, you should consider it compromised, even if it was deleted immediately.
Find here more information about risks.


🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.

Our GitHub checks need improvements? Share your feedbacks!

Copy link
Contributor

@kristianfreeman kristianfreeman left a comment

Choose a reason for hiding this comment

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

nice, this is looking a lot better!

two things i noticed

  1. formatting of the tab headers is a little messed up on low-width/mobile displays

Screen Shot 2022-07-21 at 12 45 31 PM

  1. there appears to be a border or something at the bottom - i thought it was a scrollbar at first but i don't think it is

Screen Shot 2022-07-21 at 12 45 58 PM

assets/events.ts Outdated
export function activeTab() {
var header = document.getElementById("tab-active");
var tabs = header.getElementsByClassName("tab-label");
console.log(tabs)
Copy link
Contributor

Choose a reason for hiding this comment

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

remove

assets/events.ts Outdated
for (let i = 0; i < labels.length; i++)
labels[i].addEventListener("click", $tab);
} else {
console.log({ wrappers, tabs, labels });
Copy link
Contributor

Choose a reason for hiding this comment

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

remove

@Ekwuno Ekwuno requested a review from kristianfreeman July 25, 2022 11:24
Copy link
Contributor

@kristianfreeman kristianfreeman left a comment

Choose a reason for hiding this comment

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

  1. Extra spacing at the end of the header row - should have some CSS or something to target the last tab selector and make it full-width to fill the rest of the space.

  2. Order is weird - should probably be JS SW/ESM, then TS SW/ESM.

  3. Can we add some more spacing between the icons and language label?

Screen Shot 2022-07-25 at 8 49 00 AM

Copy link
Contributor

@Cherry Cherry left a comment

Choose a reason for hiding this comment

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

Would it be possible to get a title on the JS/TS SVGs too? Otherwise to a screen-reader, each pair of 2 tabs look identical:

@Ekwuno
Copy link
Contributor Author

Ekwuno commented Jul 25, 2022

  1. Extra spacing at the end of the header row - should have some CSS or something to target the last tab selector and make it full-width to fill the rest of the space.
  2. Order is weird - should probably be JS SW/ESM, then TS SW/ESM.
  3. Can we add some more spacing between the icons and language label?
Screen Shot 2022-07-25 at 8 49 00 AM
  1. Are you viewing this on safari? The extra spacing is added by the browser. It doesn't happen on chrome or firefox; I am a bit confused about how to address it.
    Screenshot 2022-07-25 at 15 26 37

  2. The order is based on the shortcode used for this test. In useage, if you called JS SW/ESM, then TS SW/ESM it will follow that order. I could add some order to the CSS, but I don’t want that to be hard coded order so that in usage, if it doesn’t follow that order screenreaders aren’t confused. Something like this will solve it. Let me know your thoughts?

ul {
  display: flex;
  flex-direction: column;
}
ul li:first-child {
  order: 1;
}
ul li:nth-child(2) {
  order: 2;
}
ul li:nth-child(3) {
  order: 3;
}
ul li:nth-child(4) {
  order: 4;
}
  1. More space between the icons makes the text roll over, which looks a bit weird to me. I tried to make the text eligible and the icon noticeable, so the text right now is in a good spot IMO.

@Ekwuno
Copy link
Contributor Author

Ekwuno commented Jul 25, 2022

Would it be possible to get a title on the JS/TS SVGs too? Otherwise to a screen-reader, each pair of 2 tabs look identical:

Thanks for pointing this out @Cherry 🙌🏾

@Ekwuno Ekwuno requested a review from kristianfreeman July 25, 2022 17:07
@kristianfreeman
Copy link
Contributor

  1. Extra spacing at the end of the header row - should have some CSS or something to target the last tab selector and make it full-width to fill the rest of the space.
  2. Order is weird - should probably be JS SW/ESM, then TS SW/ESM.
  3. Can we add some more spacing between the icons and language label?
Screen Shot 2022-07-25 at 8 49 00 AM
  1. Are you viewing this on safari? The extra spacing is added by the browser. It doesn't happen on chrome or firefox; I am a bit confused about how to address it.
    Screenshot 2022-07-25 at 15 26 37
  2. The order is based on the shortcode used for this test. In useage, if you called JS SW/ESM, then TS SW/ESM it will follow that order. I could add some order to the CSS, but I don’t want that to be hard coded order so that in usage, if it doesn’t follow that order screenreaders aren’t confused. Something like this will solve it. Let me know your thoughts?
ul {
  display: flex;
  flex-direction: column;
}
ul li:first-child {
  order: 1;
}
ul li:nth-child(2) {
  order: 2;
}
ul li:nth-child(3) {
  order: 3;
}
ul li:nth-child(4) {
  order: 4;
}
  1. More space between the icons makes the text roll over, which looks a bit weird to me. I tried to make the text eligible and the icon noticeable, so the text right now is in a good spot IMO.

#1/#3 - that's fine with me, disregard. For #2 if it's based on how we actually write the syntax that's great, I don't think we should add a hardcoded order. Can we fix the order here just so it merges in correctly? It's confusing as written right now.

+1 from me once we land that ^ change

@Ekwuno
Copy link
Contributor Author

Ekwuno commented Jul 25, 2022

Oh, I see what you mean! So I added the shortcode to the HTML form tutorial so that we see how it is displayed. After all the reviews, I will delete it from that tutorial because it's not needed there.

{{<tabs labels="js/sw | js/esm | ts/sw | ts/esm">}} manages the order so that's sorted.

@kristianfreeman
Copy link
Contributor

sounds good. can we remove it then and i'll +1

@WalshyDev
Copy link
Member

Some future feature requests (Not needed for MVP but worth noting):

  1. Add selection into query string so we can share pages in TS+ESM for example
  2. Save your selection into localStorage and switch to that tab when you visit the page (so you will always see TS+ESM for example)

Copy link
Contributor

@Cherry Cherry left a comment

Choose a reason for hiding this comment

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

Thanks for pointing this out @Cherry 🙌🏾

Sorry for being overly picky, but now it's resulting in duplicate text for the tabs to a screen reader:

@Ekwuno
Copy link
Contributor Author

Ekwuno commented Jul 25, 2022

Some future feature requests (Not needed for MVP but worth noting):

  1. Add selection into query string so we can share pages in TS+ESM for example
  2. Save your selection into localStorage and switch to that tab when you visit the page (so you will always see TS+ESM for example)

We have a follow-up ticket for 2.

@Ekwuno Ekwuno merged commit 686cf1c into production Jul 27, 2022
@Ekwuno Ekwuno deleted the add-tab-switcher-shortcode-comp branch July 27, 2022 15:49
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.

7 participants