fix(docs): Fixing doc navigation link issues #2760

msrikanth508 · 2018-05-03T07:02:15Z

Fixed #2737 issue. Please review and let me know your comments.

welcome · 2018-05-03T07:02:17Z

💖 Thanks for opening this pull request! 💖

Here is a list of things that will help get it across the finish line:

Run yarn lint locally to catch formatting errors. This will fix some errors automatically, commit and push any changes.
Run yarn test locally to catch errors. This ensures all components still behave as they should.
Run yarn start to run the doc site locally and try a few pages, ensuring everything is in good working order.
Include tests when adding/changing behavior.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

codecov-io · 2018-05-03T07:07:22Z

Codecov Report

Merging #2760 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #2760   +/-   ##
=======================================
  Coverage   99.74%   99.74%           
=======================================
  Files         160      160           
  Lines        2762     2762           
=======================================
  Hits         2755     2755           
  Misses          7        7

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 447f96b...1484d39. Read the comment docs.

levithomason · 2018-05-05T15:37:59Z

docs/app/Components/ComponentDoc/ComponentExample/ComponentExample.js

@@ -441,7 +441,7 @@ class ComponentExample extends Component {
    }

    return (
-      <Visibility once={false} onTopPassed={this.handlePass} onTopPassedReverse={this.handlePass}>


The right menu no longer updates as the user scrolls. These handlers enabled that functionality.

Oh! I didn't check that. Thank you for pointing it out. I'll look into it.
And, all your comments make sense to me, I'll fix them.

levithomason · 2018-05-05T15:45:19Z

docs/app/Components/ComponentDoc/ComponentSidebar/ComponentSidebarSection.js

@@ -52,7 +62,7 @@ class ComponentSidebarSection extends Component {
        <Accordion.Content as={Menu.Menu} active={active}>
          {_.map(examples, ({ title, path }) => (
            <ComponentSidebarItem
-              active={activePath === path}
+              active={activePath === _.kebabCase(path.split('/').join(' '))}


The utility function would be used here as well.

levithomason · 2018-05-05T15:45:48Z

docs/app/Components/ComponentDoc/ComponentSidebar/ComponentSidebarSection.js

+
+  isActiveAccordion = (props = this.props) =>
+    (props.examples || []).findIndex(
+      item => _.kebabCase(item.path.split('/').join(' ')) === props.activePath,


The utility function would be used here.

levithomason · 2018-05-05T15:46:23Z

docs/app/Components/ComponentDoc/ComponentDoc.js

@@ -68,14 +76,28 @@ class ComponentDoc extends Component {

  handleSidebarItemClick = (e, { path }) => {
    const { history } = this.props
-    const aPath = _.kebabCase(_.last(path.split('/')))
+    const aPath = _.kebabCase(path.split('/').join(' '))


Let's pull this into a utility function in docs/apps/utils. Something like createComponentHash(). It would be ideal to be able to pass an example filename or file path to a single function that knows how to create the hash. This way it can be used everywhere we are constructing the hash.

Also, we need to add redirects from the old hashes to the new hashes. A utility function could be used for transforming the old hashes to the new hashes as well.

levithomason · 2018-05-05T15:46:36Z

docs/app/Components/ComponentDoc/ComponentExample/ComponentExample.js

@@ -71,7 +71,7 @@ class ComponentExample extends Component {
    const { examplePath, location } = this.props
    const sourceCode = this.getOriginalSourceCode()

-    this.anchorName = _.kebabCase(_.last(examplePath.split('/')))
+    this.anchorName = _.kebabCase(examplePath.split('/').join(' '))


This would be the utility function as well.

levithomason · 2018-05-05T15:51:05Z

docs/app/Components/ComponentDoc/ComponentDoc.js

+    this.state = {
+      activePath,
+    }
+  }


Prefer property initializer for state unless props are needed to calculate state:

state = { activePath: _.last((location.hash || '').split('#')) }

levithomason · 2018-05-05T16:04:40Z

We have some unnecessary duplication in the hash. Let's minimize these in this PR. We can remove the pathname and ${component}-example:

Current

/collections/table# | ${pathname}       | ${section} | ${exampleName}
                    |                   |            |          
/collections/table# | collections-table | variations | table-example-collapsing

Proposed

/collections/table# | ${section} | ${exampleName without "component-example"}
                    |            |          
/collections/table# | variations | collapsing

Comparison

/collections/table#collections-table-variations-table-example-collapsing
/collections/table#variations-collapsing

msrikanth508 · 2018-05-08T13:57:40Z

Regarding unnecessary duplication in the hash, do you want me to rename examplePath property in ComponentExample examples folder or through util function?

As we are proposing minimized hash string, we should make sure that there won't be any collisions. And, I'm wondering about redirection utility, I think there is no way to map from old hash to new hash as there is no single source of truth. For ex, old hash is /collections/table#table-example-collapsing
new hash would be /collections/table#collections-table-variations-table-example-collapsing -> collections/table#variations-collapsing, here we won't know that old hash belongs to variations category.

levithomason · 2018-05-08T18:01:58Z

Regarding unnecessary duplication in the hash, do you want me to rename examplePath property in ComponentExample examples folder or through util function?

I don't quite understand this one. The examplePath is set to match the directory structure and should remain as-is. The folder structure, examplePath values, and docgenInfo.json will all remain the same. We'll only use the util function when creating hashes. I might need more info here.

As we are proposing minimized hash string, we should make sure that there won't be any collisions.

We only have duplications when there is an example in more than one section for the same prop. By adding the section (type, state, variation, etc) to the hash we are avoiding any duplicates. Any duplicate examples for the same prop within the same category are true duplicate examples and should be merged.

I think there is no way to map from old hash to new hash as there is no single source of truth

DocsLayout might be a good place to handle this. Have a look at resetPage. We would convert old hashes to new ones here.

For ex, old hash is /collections/table#table-example-collapsing new hash would be /collections/table#collections-table-variations-table-example-collapsing -> collections/table#variations-collapsing, here we won't know that old hash belongs to variations category.

The old hashes are just the example's filename. We can _.startCase(hash).replace(/ /g, '') to get the filename again. Then, we can look it up in the exampleContext to see what section it is from. This gives us all the necessary parts to make the new hash. If we cannot lookup the hash in the exampleContext then we know it is not an old hash name and we do nothing.

We will not be able to resolve old and conflicting hashes (e.g. table collapsing) to know which section is correct (e.g. variations or types) but we can resolve all others correctly. Even in the case of conflicting example names, we will still resolve it to the new location, we just will have the wrong section sometimes.

Regarding the new hash. We should be able to create collections/table#variations-collapsing on the first pass without creating /collections/table#collections-table-variations-table-example-collapsing as a prerequisite.

msrikanth508 · 2018-05-08T18:46:34Z

I don't quite understand this one. The examplePath is set to match the directory structure and should remain as-is. The folder structure, examplePath values, and docgenInfo.json will all remain the same. We'll only use the util function when creating hashes. I might need more info here.

I got it, will create util func.

Regarding the new hash. We should be able to create collections/table#variations-collapsing on the first pass without creating /collections/table#collections-table-variations-table-example-collapsing as a prerequisite.

I meant the same, will replace current logic to generate proposed hash. I mentioned /collections/table#collections-table-variations-table-example-collapsing -> collections/table#variations-collapsing for giving more context :)

levithomason · 2018-05-11T14:53:21Z

Got it, looking forward to it.

msrikanth508 · 2018-05-12T10:41:47Z

@levithomason I've fixed code review comments and also created necessary utility functions to

Shorten hash name
Create new hash from old
Check whether give hash is old or new

msrikanth508 · 2018-05-16T06:26:45Z

@levithomason could you please review this PR?

levithomason · 2018-05-18T01:08:46Z

Apologies for the delay. I've merged master and I am reviewing this now.

welcome · 2018-05-19T00:12:53Z

Congrats on merging your first pull request! 🎉🎉🎉

levithomason · 2018-05-25T06:34:07Z

Released in semantic-ui-react@0.80.1.

fix(docs): Fixing doc navigation link issues

3c453f3

levithomason requested changes May 5, 2018

View reviewed changes

levithomason added the needs author feedback label May 5, 2018

Fixed code review comments

a800e84

Merge branch 'master' into sm-doc-links-issue

55cb9b2

levithomason added 3 commits May 17, 2018 19:13

refactor(docs): split up util functions

46ec7dc

fix(ComponentControls): show menu when deep linking

21069f4

fix(docs): handle ComponentExample hash names

684082f

levithomason force-pushed the sm-doc-links-issue branch from b27f077 to 684082f Compare May 18, 2018 02:36

levithomason added 2 commits May 17, 2018 19:37

fix(docs): export getNewHash in utils

561ef58

consolidate hash functions and usage

1484d39

levithomason approved these changes May 18, 2018

View reviewed changes

levithomason added ready for merge and removed needs author feedback labels May 18, 2018

levithomason merged commit 14b7eca into Semantic-Org:master May 19, 2018

msrikanth508 deleted the sm-doc-links-issue branch May 22, 2018 17:23

layershifter mentioned this pull request Mar 22, 2019

fix(docs): remove component page hash replacement #3528

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(docs): Fixing doc navigation link issues #2760

fix(docs): Fixing doc navigation link issues #2760

msrikanth508 commented May 3, 2018

welcome bot commented May 3, 2018

codecov-io commented May 3, 2018 •

edited

Loading

levithomason May 5, 2018

msrikanth508 May 5, 2018

levithomason May 5, 2018

levithomason May 5, 2018

levithomason May 5, 2018

levithomason May 5, 2018

levithomason May 5, 2018

levithomason commented May 5, 2018

msrikanth508 commented May 8, 2018

levithomason commented May 8, 2018

msrikanth508 commented May 8, 2018

levithomason commented May 11, 2018

msrikanth508 commented May 12, 2018

msrikanth508 commented May 16, 2018

levithomason commented May 18, 2018

welcome bot commented May 19, 2018

levithomason commented May 25, 2018

fix(docs): Fixing doc navigation link issues #2760

fix(docs): Fixing doc navigation link issues #2760

Conversation

msrikanth508 commented May 3, 2018

welcome bot commented May 3, 2018

codecov-io commented May 3, 2018 • edited Loading

Codecov Report

levithomason May 5, 2018

Choose a reason for hiding this comment

msrikanth508 May 5, 2018

Choose a reason for hiding this comment

levithomason May 5, 2018

Choose a reason for hiding this comment

levithomason May 5, 2018

Choose a reason for hiding this comment

levithomason May 5, 2018

Choose a reason for hiding this comment

levithomason May 5, 2018

Choose a reason for hiding this comment

levithomason May 5, 2018

Choose a reason for hiding this comment

levithomason commented May 5, 2018

msrikanth508 commented May 8, 2018

levithomason commented May 8, 2018

msrikanth508 commented May 8, 2018

levithomason commented May 11, 2018

msrikanth508 commented May 12, 2018

msrikanth508 commented May 16, 2018

levithomason commented May 18, 2018

welcome bot commented May 19, 2018

levithomason commented May 25, 2018

codecov-io commented May 3, 2018 •

edited

Loading