Feat: Page per section upgraded with page not found #993

rafaesc · 2018-05-25T21:00:57Z

This PR is a feature of this issue:
#892

And started with this branch
#989

Also added a page not found

codecov-io · 2018-05-25T21:10:56Z

Codecov Report

Merging #993 into master will decrease coverage by 0.04%.
The diff coverage is 97.53%.

Impacted Files	Coverage Δ
...omponents/ComponentsList/ComponentsListRenderer.js	`100% <ø> (ø)`	⬆️
src/utils/getPageTitle.js	`100% <100%> (ø)`	⬆️
src/rsg-components/slots/index.js	`100% <100%> (ø)`	⬆️
...rc/rsg-components/SectionHeading/SectionHeading.js	`100% <100%> (ø)`	⬆️
src/utils/filterComponentsInSectionsByExactName.js	`100% <100%> (ø)`	⬆️
src/rsg-components/Examples/Examples.js	`100% <100%> (ø)`	⬆️
...rc/rsg-components/ComponentsList/ComponentsList.js	`100% <100%> (ø)`	⬆️
src/rsg-components/Playground/Playground.js	`93.54% <100%> (+0.69%)`	⬆️
.../rsg-components/TableOfContents/TableOfContents.js	`95% <100%> (+0.88%)`	⬆️
scripts/schemas/config.js	`88.4% <100%> (+0.9%)`	⬆️
... and 15 more

This reverts commit fa145b3.

sapegin

Thanks!

Can we close #989 then?

sapegin · 2018-05-27T16:20:10Z

src/index.js

@@ -10,8 +10,20 @@ let codeRevision = 0;

 /** Scrolls to origin when current window location hash points to an isolated view. */
 const scrollToOrigin = () => {
-	if (window.location.hash.indexOf('#!/') === 0) {
-		window.scrollTo(0, 0);
+	const hash = window.location.hash;


This code needs a lot of comments, meaningful variable names and splitting into multiple functions with meaningful names. It's impossible to understand what it does.

sapegin · 2018-05-27T16:23:39Z

src/rsg-components/NotFound/NotFoundRenderer.js

+	root: {
+		maxWidth,
+		margin: [[0, 'auto']],
+		padding: space[4],


This padding creates extra whitespace, that is visible on the screenshot.

sapegin · 2018-05-27T16:24:14Z

src/rsg-components/NotFound/NotFoundRenderer.js

+				text={`
+# 404
+
+## Sorry, this page isn't available


# Page not found would be enough instead of both headings.

sapegin · 2018-05-27T16:25:01Z

src/rsg-components/NotFound/NotFoundRenderer.js

+
+## Sorry, this page isn't available
+
+The link you followed may be broken, or the page may have been removed


Please add . at the end ;-)

sapegin · 2018-05-27T16:25:59Z

src/rsg-components/Playground/Playground.js

@@ -6,14 +6,15 @@ import Para from 'rsg-components/Para';
 import Slot from 'rsg-components/Slot';
 import PlaygroundRenderer from 'rsg-components/Playground/PlaygroundRenderer';
 import { EXAMPLE_TAB_CODE_EDITOR } from '../slots';
-import { DisplayModes } from '../../consts';
+import { DisplayModes, ExampleMode } from '../../consts';


DisplayModes, but ExampleMode. Was that intentional?

sapegin · 2018-05-27T16:33:54Z

src/utils/getInfoFromHash.js

-		const index = parseInt(tokens[1], 10);
+	const shouldIsolate = hash.substr(0, 3) === '#!/';
+	if (shouldIsolate || hash.substr(0, 2) === '#/') {
+		const path = hash.replace(/\?id=[^]*/, '');


This is very fragile, what if there will be &id=? It will also remove all parameters after id.

sapegin · 2018-05-27T16:35:26Z

src/utils/getInfoFromHash.js

+	const shouldIsolate = hash.substr(0, 3) === '#!/';
+	if (shouldIsolate || hash.substr(0, 2) === '#/') {
+		const path = hash.replace(/\?id=[^]*/, '');
+		const tokens = path


We can split the first part at the very beginning with a regexp and avoid all substr calls and magic numbers.

I’m wondering why not introduce a routing solution to the project? It would simplify thing a lot since we won’t need parse location ourselves I guess.

@okonet Like history js?, but its possible that it necessary migrate several things.

I don't think we need a router like React Router because we render a single component in any case except welcome screen. But using a library to parse URLs could be a good idea to avoid hard-to-read string operations and regexps.

Yeah I agree complete router might be an overkill but let’s not do parsing ourselves

sapegin · 2018-05-27T16:36:46Z

src/utils/getInfoFromHash.js

-	if (hash.substr(0, 3) === '#!/') {
-		const tokens = hash.substr(3).split('/');
-		const index = parseInt(tokens[1], 10);
+	const shouldIsolate = hash.substr(0, 3) === '#!/';


This function also needs splitting and comments.

sapegin · 2018-05-27T16:37:12Z

src/utils/getPageTitle.js

-	} else if (displayMode === DisplayModes.section) {
-		return `${sections[0].name} — ${baseTitle}`;
+	if (displayMode === DisplayModes.notFound) {
+		return 'Error 404 - ' + baseTitle;


Error 404 → Page not found

sapegin · 2018-05-27T16:39:15Z

src/utils/getRouteData.js

-			displayMode = DisplayModes.component;
+		let filteredSections;
+		if (pagePerSection) {
+			tokens.forEach((tokenName, index) => {


What is happening here?

okonet

I agree with all Artem’s comments

rafaesc · 2018-05-29T22:33:05Z

@sapegin @okonet I added a new file to handle the hash.

https://github.com/styleguidist/react-styleguidist/blob/ad5b8b2cf0cf6e4f17f4af34cd3031fc284f992e/src/utils/handleHash.js

You can see the tests.

https://github.com/styleguidist/react-styleguidist/blob/ad5b8b2cf0cf6e4f17f4af34cd3031fc284f992e/src/utils/__tests__/handleHash.spec.js

And his implementation here

react-styleguidist/src/utils/getInfoFromHash.js

Lines 17 to 29 in ad5b8b2

    
           export default function getInfoFromHash(hash) { 
        
           	const shouldIsolate = hasInHash(hash, '#!/'); 
        
           	if (shouldIsolate || hasInHash(hash, '#/')) { 
        
           		const hashArray = getHashAsArray(hash, shouldIsolate ? '#!/' : '#/'); 
        
           		const index = parseInt(hashArray[hashArray.length - 1], 10); 
        
           		return { 
        
           			isolate: shouldIsolate, 
        
           			hashArray: hashArray.filter(filterNumbers), 
        
           			targetName: hashArray[0], 
        
           			targetIndex: isNaN(index) ? undefined : index, 
        
           		}; 
        
           	} 
        
           	return {};

react-styleguidist/src/index.js

Lines 12 to 31 in ad5b8b2

    
           /** Scrolls to origin when current window location hash points to an isolated view. */ 
        
           const scrollToOrigin = () => { 
        
           	const hash = window.location.hash; 
        
           	if (hasInHash(hash, '#/') || hasInHash(hash, '#!/')) { 
        
           		const element = document.scrollingElement || document.documentElement; // cross-browsers 
        
           		/** Extracts the id param of hash */ 
        
           		const idHashParam = getParameterByName(hash, 'id'); 
        
           		let scrollTop = 0; 
        
           		if (idHashParam) { 
        
           			// Searches the node with the same id 
        
           			const idElement = document.getElementById(idHashParam); 
        
           			if (idElement && idElement.offsetTop) { 
        
           				scrollTop = idElement.offsetTop; 
        
           			} 
        
           		} 
        
           		/** Scroll to node or page top */ 
        
           		element.scrollTop = scrollTop; 
        
           	} 
        
           };

sapegin · 2018-05-30T07:54:44Z

I still think we should try something like https://github.com/unshiftio/url-parse instead of doing this manually.

rafaesc · 2018-05-31T17:51:56Z

@sapegin I was thinking where use url-parse but I am not sure if we really need it because url-parse doesn't have a method to get the pages of hash and our router is something particular.

I found path-to-regexp, it seems useful but it is more useful to set an URL with params
https://github.com/pillarjs/path-to-regexp/blob/master/Readme.md#compile-reverse-path-to-regexp

Then found hasher, it's a good library because you can customized the pretend hash and you can get the router as array, but it has a strong dependency with location.hash, so it needs to integrate the router with the library when we only need an util.

So I had decided to adapt the part of hasher that we need in handleHash file, it doesn't have a strong dependency, easy to test and isn't necessary a big migration.

What do you think?

okonet · 2018-06-02T18:00:40Z

Reach Router is tiny so I’m wondering if we should try using it. It should simply a lot of code imo. https://mobile.twitter.com/ryanflorence/status/1002959657235722240

sapegin · 2018-06-03T16:24:19Z

If it really simplify code (keep in mind that we’re rendering the same component with different props depending on the URL), then I’m fine with it. But we’ll still need something to parse query strings.

okonet · 2018-06-03T16:36:46Z

Let’s try router in a separate PR and finish this one. Do we have a conclusion?

sapegin · 2018-06-03T16:37:59Z

Yup, let’s keep this PR as is, the code is isolated at least ;-/

rafaesc · 2018-06-03T17:16:14Z

@sapegin @okonet thanks!

I will add documentation

rafaesc · 2018-06-06T18:50:08Z

@sapegin it's strange that Travis told that there is an error when I only modified Markdown files.

sapegin · 2018-06-06T18:58:34Z

Restart usually helps, any CI can fail for many reasons not related to code changes ;-)

rafaesc · 2018-06-06T19:19:04Z

Yes, yesterday I restarted twice and it had the same result

Opening Cypress...
A configuration option you have supplied has been renamed.
Please rename videoRecording to video

maybe I have to send another change to work fine, please @sapegin review my changes in docs.

sapegin · 2018-06-07T12:37:38Z

That should be fixed in master already.

sapegin · 2018-06-17T11:02:22Z

docs/Configuration.md

@@ -282,6 +292,47 @@ module.exports = {
 }
 ```

+You can set the number of subpages of each section by adding`sectionDepth` in each first level section.


I don't understand what that means ;-(

sapegin · 2018-06-17T11:04:04Z

loaders/utils/getSections.js

@@ -48,12 +51,19 @@ function processSection(section, config) {
 		}
 		content = requireIt(`!!${examplesLoader}!${contentAbsolutePath}`);
 	}
+	sectionDepth =


This needs some explanation: what is happening here? What is firstDepth?

sapegin · 2018-06-17T11:05:36Z

src/index.js

+	const hash = window.location.hash;
+	if (hasInHash(hash, '#/') || hasInHash(hash, '#!/')) {
+		const element = document.scrollingElement || document.documentElement; // cross-browsers
+		/** Extracts the id param of hash */


Please don't use block comments instead of inline comments, also add an empty line before a comment to separate code paragraphs.

sapegin · 2018-06-17T11:06:22Z

src/index.js

-		window.scrollTo(0, 0);
+	const hash = window.location.hash;
+	if (hasInHash(hash, '#/') || hasInHash(hash, '#!/')) {
+		const element = document.scrollingElement || document.documentElement; // cross-browsers


What this line is doing? What this comment means? Which browsers? Worth extracting to a method with a meaningful name.

sapegin · 2018-06-17T11:09:37Z

src/utils/getPageTitle.js

-	} else if (displayMode === DisplayModes.section) {
-		return `${sections[0].name} — ${baseTitle}`;
+	if (displayMode === DisplayModes.notFound) {
+		return 'Error 404 - Page not found';


Just “Page not found", I think I've mentioned this before.

sapegin · 2018-06-17T11:10:28Z

src/utils/getRouteData.js

+
+		if (pagePerSection) {
+			// Try to filter each section for depth of each hash ["Documentation", "Files", "Button"]
+			// When sectionDepth is major of 0, their children should be filtered


What “major of 0” means?

sapegin · 2018-06-17T11:11:26Z

src/utils/getRouteData.js

+			if (!sections.length) {
+				displayMode = DisplayModes.notFound;
+			}
+			// The targetName takes the last of hashArray


This comment is repeating the code, but doesn't explain it.

sapegin · 2018-06-17T11:12:36Z

src/utils/getUrl.js

 ) {
 	let url = pathname;

+	if (takeHash) {
+		url += hash.replace(/\?id=[^]*/, '');


I've mentioned before that this is very destructive regexp, it will cause issues.

sapegin · 2018-06-17T11:13:57Z

src/utils/handleHash.js

+const separator = '/';
+const hashValRegexp = /(.*)\?/;
+
+function escapeRegExp(str) {


Use escapeRegExp from Lodash.

sapegin · 2018-06-17T11:14:52Z

src/utils/handleHash.js

@@ -0,0 +1,73 @@
+const defaultPrependHash = '#/';


This fine needs a huge “HACK” comment explaining that this is a temporary solution.

rafaesc · 2018-06-22T17:25:21Z

It seems that there still is the bug when Travis tries to validate the branch, however, the PR was passed by Travis.

sapegin · 2018-06-24T10:15:48Z

docs/Configuration.md

@@ -292,7 +292,7 @@ module.exports = {
 }
 ```

-You can set the number of subpages of each section by adding`sectionDepth` in each first level section.
+If you want to isolate section's children as single pages, you can set up it adding `sectionDepth` in each section. Where `sectionDepth` it is the number of depth of routes that it will have each section.


you can set up it adding sectionDepth in each section… → add the sectionDepth parameter to a section, where the value is the number of section levels that will be shown as single pages?

Though I’m still not 100% sure I understand now it works ;-/

sapegin · 2018-06-24T10:16:52Z

docs/Configuration.md

    }
  ]
 }
 ```

+For default `sectionDepth`


Should this be removed or finished?

sapegin · 2018-06-24T10:18:27Z

src/index.js

 		let scrollTop = 0;

 		if (idHashParam) {
-			// Searches the node with the same id
+			// Searches the node with the same id and takes his offsetTop


The comment repeats the code but doesn't explain the intent.

sapegin · 2018-06-24T10:22:01Z

Yeah, that's weird. I don't understand the difference between push and pr anyway ;-/

I think the code is fine now, just a few docs tweaks left.

sapegin · 2018-07-01T10:55:24Z

Could you please merge master? Is there anything else left or we can make 7.1 release with this?

rafaesc · 2018-07-01T15:06:27Z

@sapegin I think there is no more, I am going to solve the conflict with the master branch

sapegin · 2018-07-02T15:35:01Z

Merging this to maser wasn't a good idea, I'm going to revert it.

rafaesc · 2018-07-02T15:41:52Z

Apologies I thought you mean that, tell me if I can help in any way

sapegin · 2018-07-02T15:47:26Z

Maybe I wasn't clear enough, I meant to merge master into your branch to resolve merge conflicts.

I've reverted the merge now, but you will need to open a new PR I think (renaming the branch and opening a new PR should work).

P. S. You can try to make a new release if you want, there are some docs about the process. Let me know if that's something you'd like to do ;-)

rafaesc · 2018-07-02T15:55:25Z

Perfect, thanks!

I will work it :)

sapegin · 2018-07-02T15:56:46Z

Ping me if you have any questions, there's a milestone for the next release.

styleguidist-bot · 2018-07-14T18:12:31Z

🎉 This PR is included in version 7.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

rafaesc added 8 commits May 16, 2018 18:59

Feat: Added exampleModes and usageModes settings per sections

04e36e6

Test: Updated snap

fd8f119

Fix: Support showCode and showUsage

039cfcf

Fix: Name correction and updated config

2f5431c

Feat: pagePerSection updated with sectionDepth

5b8144e

Fix: sectionDepth is validated only in the first depth

1afcd5e

Feat: Added not found page

e3c0c97

Feat: Added links in .md examples

e61caf1

rafaesc added 2 commits May 25, 2018 16:56

Test: Added tests

d0a0a0c

Test: Added getUrl tests

fa145b3

rafaesc mentioned this pull request May 25, 2018

Separate pages & URLs for all nav items to allow richer documentation of components arielsalminen/vue-design-system#63

Closed

Revert "Test: Added getUrl tests"

a1f3903

This reverts commit fa145b3.

sapegin requested a review from okonet May 27, 2018 16:41

sapegin requested changes May 27, 2018

View reviewed changes

okonet reviewed May 27, 2018

View reviewed changes

Fix: Updated with reviews

ad5b8b2

Docs: Added pagePerSection docs

393f910

sapegin mentioned this pull request Jun 17, 2018

Styleguide crashes when accessing invalid isolated urls #1016

Closed

sapegin requested changes Jun 17, 2018

View reviewed changes

Fix: code review of page per sections

e0842ff

sapegin requested changes Jun 24, 2018

View reviewed changes

Doc: Updated of according code reviews

aa34bfe

sapegin approved these changes Jul 1, 2018

View reviewed changes

rafaesc merged commit aa34bfe into master Jul 2, 2018

sapegin added this to the 7.1.0 milestone Jul 2, 2018

sapegin removed this from the 7.1.0 milestone Jul 6, 2018

Abdurahmanov mentioned this pull request Jul 18, 2018

Adding a reach-router #1056

Closed

sapegin deleted the feat-page-per-section branch November 11, 2020 16:32

Yoasia1985 mentioned this pull request Jul 7, 2023

[Snyk] Security upgrade babel-loader from 8.0.5 to 9.1.3 Yoasia1985/react-styleguidist#77

Open

kopax mentioned this pull request Jul 8, 2023

[Snyk] Security upgrade babel-loader from 8.0.5 to 9.1.3 rollup-umd/react-styleguidist#150

Open


		## Sorry, this page isn't available

		The link you followed may be broken, or the page may have been removed

Feat: Page per section upgraded with page not found #993

Feat: Page per section upgraded with page not found #993

Conversation

rafaesc commented May 25, 2018 • edited Loading

codecov-io commented May 25, 2018 • edited Loading

Codecov Report

sapegin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

okonet left a comment

Choose a reason for hiding this comment

rafaesc commented May 29, 2018 • edited Loading

sapegin commented May 30, 2018

rafaesc commented May 31, 2018 • edited Loading

okonet commented Jun 2, 2018

sapegin commented Jun 3, 2018

okonet commented Jun 3, 2018

sapegin commented Jun 3, 2018

rafaesc commented Jun 3, 2018

rafaesc commented Jun 6, 2018

sapegin commented Jun 6, 2018

rafaesc commented Jun 6, 2018

sapegin commented Jun 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rafaesc commented Jun 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sapegin commented Jun 24, 2018

sapegin commented Jul 1, 2018

rafaesc commented Jul 1, 2018

sapegin commented Jul 2, 2018

rafaesc commented Jul 2, 2018

sapegin commented Jul 2, 2018

rafaesc commented Jul 2, 2018

sapegin commented Jul 2, 2018

styleguidist-bot commented Jul 14, 2018

rafaesc commented May 25, 2018 •

edited

Loading

codecov-io commented May 25, 2018 •

edited

Loading

rafaesc commented May 29, 2018 •

edited

Loading

rafaesc commented May 31, 2018 •

edited

Loading