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

[Docs] Consolidated tabs for Examples-Guidelines-Playground #3650

Merged
merged 8 commits into from
Jul 1, 2020

Conversation

anishagg17
Copy link
Contributor

@anishagg17 anishagg17 commented Jun 23, 2020

Summary

Fixes: #3649

Checklist

- [ ] Check against all themes for compatibility in both light and dark modes
- [ ] Checked in mobile
- [ ] Checked in IE11 and Firefox
- [ ] Props have proper autodocs
- [ ] Added documentation
- [ ] Checked Code Sandbox works for the any docs examples
- [ ] Added or updated jest tests
- [ ] Checked for breaking changes and labeled appropriately

  • Checked for accessibility including keyboard-only and screenreader modes
    - [ ] A changelog entry exists and is marked appropriately

@kibanamachine
Copy link

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@anishagg17 anishagg17 changed the title tabs for Examples-Guidelines-Playground [Docs] Consolidate tabs for Examples-Guidelines-Playground Jun 23, 2020
@anishagg17 anishagg17 changed the title [Docs] Consolidate tabs for Examples-Guidelines-Playground [Docs] Consolidated tabs for Examples-Guidelines-Playground Jun 23, 2020
@snide
Copy link
Contributor

snide commented Jun 23, 2020

jenkins test this

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_3650/

@thompsongl
Copy link
Contributor

Woah that was quick, @anishagg17!
Looking good after a spot check. I'll take a look at the code tomorrow morning.

@anishagg17
Copy link
Contributor Author

What about

<Link to="/guidelines/forms">forms usage guidelines</Link>.
this link

@thompsongl
Copy link
Contributor

this link

Brings up a good question. Ideally I think we'd want a permalink system to navigate directly to a specific tab inside of a component page. Without committing any code yet, does react-router have a way to handle this?

@anishagg17
Copy link
Contributor Author

anishagg17 commented Jun 25, 2020

does react-router have a way to handle this

Yes , I have used react-router to fix it but the only query arises with the fallback

If user goes to navigation/button/guidelines/something what should be the output guidelines page or the examples page ?

Also , I wanted to know if intro should be displayed with all tabs or the examples tab only?

@thompsongl
Copy link
Contributor

If user goes to navigation/button/guidelines/something what should be the output guidelines page or the examples page ?

What's the something in this case? A sub-link in the nav sidebar below "Button"?

Also , I wanted to know if intro should be displayed with all tabs or the examples tab only?

Intro is only for Examples

@anishagg17
Copy link
Contributor Author

What's the something in this case? A sub-link in the nav sidebar below "Button"?

No, i mean if we use react-router then users can alter the link themselves and it might appear like navigation/button/guidelines/anything

navigation/button/guidelines/ (exact) would show the content of guidelines page.
Should any change be ignored ,i.e, both navigation/button/guidelines/anything and navigation/button/guidelines/ should display the content of guidelines page.

@thompsongl
Copy link
Contributor

Ah, right. Let's ignore everything after guidelines/ for now.

@anishagg17
Copy link
Contributor Author

Should I push the commit now then?

@thompsongl
Copy link
Contributor

Sure, let's try it out

@anishagg17
Copy link
Contributor Author

Updated 🤗

@anishagg17
Copy link
Contributor Author

Just a small question , Should we supply only the configuration file of each playground to the GuidePage (and thereby creating it's UI) instead of creating whole UI for the playground and then supplying the component to the GuidePage as it would save a lot of code.

For example : we are supplying the config file for examples

@thompsongl
Copy link
Contributor

Not sure I fully understand.

Let's take https://github.com/elastic/eui/pull/3650/files#diff-da3173d0dbf7111093e736dc32c71f90R389 as an example (EuiButton playground). Are you suggesting we provide some kind of config object here instead of a component?
If so, what does that config look like?

This also may be a question that can be addressed later. This PR will merge without any playground examples showing, so as long as the choice between config or component does not dramatically alter the tab system technically, it may be best to focus on that problem individually in a separate PR.

@anishagg17
Copy link
Contributor Author

This PR will merge without any playground examples showing, so as long as the choice between config or component does not dramatically alter the tab system technically, it may be best to focus on that problem individually in a separate PR.

Yes , my question is not related this pr, but related to future pr.

Are you suggesting we provide some kind of config object here instead of a component?

Yes , a config object instead of component as we pass for sections a bit similar.

If so, what does that config look like?

Would mostly consists of props config.

it may be best to focus on that problem individually in a separate PR.

Sure.

@thompsongl
Copy link
Contributor

jenkins test this

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_3650/

@thompsongl thompsongl requested a review from cchaos June 26, 2020 20:15
@thompsongl
Copy link
Contributor

All the basics are here, so let's get a design review from @cchaos

I'll do an eng review afterwards

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

Looks great! The mobile version could use a little cleanup but a designer can do a follow-up PR.

Since this is merging into master, we'll just want to be sure to remove the blank Playground pages before merge.

@thompsongl
Copy link
Contributor

Can we set up a forward/redirect for the old Guidelines routes?
It'd be nice for /guidelines/button to redirect to /navigation/button/guidelines and follow that pattern just for the couple pages that moved.

@anishagg17
Copy link
Contributor Author

Screenshot 2020-07-01 at 12 45 10 AM

This is the new redirect-scheme

@thompsongl
Copy link
Contributor

jenkins test this

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_3650/

Copy link
Contributor

@thompsongl thompsongl left a comment

Choose a reason for hiding this comment

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

Nice work, @anishagg17!

@anishagg17
Copy link
Contributor Author

Thanks

@thompsongl thompsongl merged commit 9f81f7f into elastic:master Jul 1, 2020
@anishagg17 anishagg17 deleted the phase1 branch July 1, 2020 16:14
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.

[Docs] Consolidate component pages into tabbed UI
5 participants