Sort properties by 'required' and 'name' attributes #784

dotcs · 2018-01-22T09:37:02Z

I've noticed that properties are not sorted. I think it would make sense to first show the required properties and then the optional ones. Each of both groups should be grouped by name.

This PR solves the issue by adjusting how propsToArray works.

codecov-io · 2018-01-22T09:48:00Z

Codecov Report

Merging #784 into master will increase coverage by 0.03%.
The diff coverage is 100%.

Impacted Files	Coverage Δ
src/rsg-components/Usage/Usage.js	`100% <ø> (ø)`	⬆️
scripts/schemas/config.js	`86.2% <ø> (ø)`	⬆️
loaders/props-loader.js	`97.72% <100%> (+0.58%)`	⬆️
src/rsg-components/Props/PropsRenderer.js	`98.24% <100%> (-0.04%)`	⬇️
loaders/utils/sortProps.js	`100% <100%> (ø)`

sapegin

Thanks! That looks useful.

I think we should do in on the backend though, in the props-loader somewhere. Ping me if you need help with finding the right place ;-)

dotcs · 2018-01-22T10:28:58Z

@sapegin I'm not completely sure, but I guess it's not possible since props is an object all the time until it's converted to an array. IMHO this is the first (and only?) position where we can sort values.

sapegin · 2018-01-22T11:56:21Z

I don't think there's a strict requirement to keep it as an object, but I'm afraid it may be too difficult to change now.

This reverts commit 182fc89.

dotcs · 2018-01-23T08:21:22Z

src/rsg-components/Props/Props.spec.js

 import { unquote, getType, showSpaces } from './util';

 // Test renderers with clean readable snapshot diffs
 // eslint-disable-next-line react/prop-types
 export default function ColumnsRenderer({ props }) {
 	return (
 		<ul>
-			{propsToArray(props).map((row, rowIdx) => (
+			{/* eslint-disable react/prop-types */}


Not sure why react/prop-types makes problems here. I've tried to add propTypes for this component but had no success. Do you have an idea?

ColumnsRenderer.propTypes = { props: PropTypes.array.isRequired, }

And it was already disabled: // eslint-disable-next-line react/prop-types ;-/ I guess because of the name props.

Oh, I didn't notice that. 👍

Let's disable it for the whole file, it's a test anyway ;-)

dotcs · 2018-01-23T08:33:29Z

@sapegin Changing how the props-loader works was a bit harder, but it's possible I guess. Please have another look. Since I don't know the code by heart I hope I have found every occurrence of props object vs. array, so please consider having another look into other components that are involved which I could have forgotten.

dotcs · 2018-01-24T09:42:05Z

@sapegin What do you think? Are there still changes necessary or are you fine with it? Current review status says that there are still requested changes.

sapegin

Couple of small remarks, otherwise looks good. Thanks!

sapegin · 2018-01-24T10:11:41Z

loaders/props-loader.js

@@ -12,6 +12,18 @@ const consts = require('../scripts/consts');

 const ERROR_MISSING_DEFINITION = 'No suitable component definition found.';

+function sortProps(props) {


This function should be in a separate file with JSDocs comment, like other functions. Will be easier to test separately too.

You're right. I've separated the function into an own file, added JSDocs and tests.

sapegin · 2018-01-24T10:12:10Z

loaders/props-loader.js

@@ -52,6 +64,10 @@ module.exports = function(source) {
 	}

 	props = getProps(props, file);
+	if (props.props) {
+		const sortedProps = sortProps(props.props);
+		props.props = sortedProps;


I think props.props = sortProps(props.props); would be enough here.

That's true. Done. 👍

sapegin · 2018-01-24T10:14:00Z

src/rsg-components/Props/Props.spec.js

 import { unquote, getType, showSpaces } from './util';

 // Test renderers with clean readable snapshot diffs
 // eslint-disable-next-line react/prop-types
 export default function ColumnsRenderer({ props }) {
 	return (
 		<ul>
-			{propsToArray(props).map((row, rowIdx) => (
+			{/* eslint-disable react/prop-types */}


Let's disable it for the whole file, it's a test anyway ;-)

dotcs · 2018-01-24T11:08:34Z

@sapegin Alright, I've fixed all findings 👍

n1313 · 2018-01-25T05:05:35Z

Can this be made optional? Props have different significance, and I personally sort/group them manually in the order of significance. For example, I would dislike having something like onTransactionSigned placed above transactions because you wouldn't understand what onTransactionSigned does without reading about transactions first.

dotcs · 2018-01-25T07:47:44Z

@n1313 I guess it could be made optional. In the props-loader we have access to the config object, which could be used to add additional config to it.
Here we could enable or disable the sorting, e.g. by having a property called sortComponentProperties: boolean. The default value could be false to keep the default as it is right now.
@sapegin What do you think?

sapegin · 2018-01-25T07:56:30Z

I'd suggest to make a more generic option to transform props with the default implementation that will sort them like in this PR. To disable sorting you could pass a function that does nothing. I don't really like the idea or an option that control sorting of props, that feels too specific.

dotcs · 2018-01-25T09:02:10Z

I've added my idea of how a generic function could look like. What are your opinions on that approach?

n1313 · 2018-01-25T09:58:28Z

The option of sorting props is valuable and useful, but I would personally prefer the default prop display behaviour to not change (especially if it is coming in with only a minor version bump).

sapegin · 2018-01-25T10:02:46Z

I'd like to collect more opinions, so far we're two against one ;-)

@okonet @kof @SaraVieira @stepancar What do you think?

dotcs · 2018-01-25T10:05:45Z

@n1313 I understand your point, but I do not agree. The current implementation does not make any assumptions about the order of the properties. Eventually it uses the sorting of keys in an object, which is not standardised (see here). So this should change anyways to make it reliable - even if this had not been an issue so far.

kof · 2018-01-25T10:06:44Z

this sort by default makes sense, evtl optionally could be disabled, but I don't care right now about this, I hate manually decide what is more important and most of the time just put require stuff first. Btw this should be an eslint feature which also works with --fix

n1313 · 2018-01-25T10:10:39Z

Well, truth be told, I'm still sitting on v5.3.2 (because I couldn't get over the previous UI change that came in with a minor version bump and undoing that change is too much effort), so this change won't affect me at all. But if I was on v6, I would be pissed.

kof · 2018-01-25T10:12:28Z

One suggestion to validate users reaction could be to release a beta first and see if any one else has hard feelings about this change. In general prettier showed that good automated defaults are well accepted by the community.

kof · 2018-01-25T10:14:54Z

Btw. having this in eslint would completely solve the problem as you would shift this decision to the user.

n1313 · 2018-01-25T10:16:22Z

https://eslint.org/docs/rules/sort-keys exists, but it doesn't work with --fix :|

dotcs · 2018-01-26T06:25:02Z

eslint does not solve this problem, because eslint is a tool for structuring code. But it does not change the way how the underlying JavaScript engine interprets objects and how it sorts the keys. react-docgen (or react-docgen-typescript) returns the property as an object in which the property names are its keys. This is what we get and what we have to work with!
As mentioned above sorting of keys in an dictionary is not standardised. This means that how the properties are sorted right now is not guaranteed in any way. eslint does not change anything - regardless of how the user sort the properties and how they get parsed by react-docgen.

The only way to solve this problem is to use a data-structure with a guaranteed order, such as arrays. This PR tries to archive an order of the properties by transforming the object to an array of objects where a strict order is defined and uses this array for property rendering.

So please keep this discussion focused. eslint does something completely different. The sort-keys rule is meant for readability not for data integrity. It does not help in this discussion in any way.

sapegin · 2018-01-26T06:31:46Z

Well, truth be told, I'm still sitting on v5.3.2 (because I couldn't get over the previous UI change that came in with a minor version bump and undoing that change is too much effort), so this change won't affect me at all. But if I was on v6, I would be pissed.

The let’s just do this and see if anyone else will complain. I believe this would be more useful for majority of the users and we have a way to disable this. We can’t make all users happy anyway.

dotcs · 2018-01-26T06:37:09Z

[...] we have a way to disable this

That's true. We could disable it by using propsTransform: props => props. This passes the array without any transformation (also no sorting) which would replicate the way it has been done before.

To completely solve the problem and not rely on the pseudo-sorting of keys in an object we would have to change the way how react-docgen and react-docgen-typescript save properties. It should be an array already at this point such that the order is guaranteed, but this is not the scope of this PR.

sapegin · 2018-01-26T06:32:54Z

docs/Configuration.md

+
+Type: `Function`, optional
+
+Function that transforms component properties.


An example how to disable sorting and note that the default implementation sorts props would be nice.

Good idea. I've improved the documentation. Please have another look.

…e configured

n1313 · 2018-01-26T07:16:20Z

let’s just do this and see if anyone else will complain.

What's the plan if someone else does complain? Roll back the release?

We can’t make all users happy anyway.

In my opinion it is important to note that current situation with props being unsorted is not a known source of unhappiness for any noticeable fraction of users of RSG (at the very least I am not aware of any issues or complaints about this in the repo). Many users are relying on current behaviour of RSG even though it is non-standard, and changing it will no doubt be a surprise to them.

Please consider keeping current behaviour as default by shipping with propsTransform: props => props and instead adding the sorting function to documentation as a useful example of using propsTransform.

sapegin · 2018-01-26T07:30:20Z

What's the plan if someone else does complain? Roll back the release?

We can always change the default value.

Please consider keeping current behaviour as default by shipping with propsTransform: props => props and instead adding the sorting function to documentation as a useful example of using propsTransform.

I did consider that and have made a decision, thanks for comments.

sapegin · 2018-01-26T07:32:00Z

docs/Configuration.md

+
+```javascript
+module.exports = {
+  transformProps: function(props) {


props => props ;-)

okonet · 2018-01-26T09:10:44Z

I think it’s a great thing to put required props on top and option below. I can imagine some push back from very pedantic users who carefully organize props in the source code so an opt-out should be provided but I believe most of users will appreciate this.

sapegin · 2018-03-20T16:37:58Z

🎉 This PR is included in version 6.4.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@bitionaire

👋 **[Support Styleguidist](https://opencollective.com/styleguidist) on Open Collective** 👋 ## New features ### Page per section 🚧 This is a huge improvement for large style guides: now you can show only one component at a page instead of all components. ![](https://d3vv6lp55qjaqc.cloudfront.net/items/0W2q2K2s3n2k311O1J0y/Screen%20Recording%202018-03-22%20at%2010.06%20AM.gif) To enable: ```js module.exports = { pagePerSection: true }; ``` This is an experimental feature and we need your feedback to make it better and really useful. For example, right now there’s no isolated mode. So feel free to share your ideas [in issues](https://github.com/styleguidist/react-styleguidist/issues). (#835 by @bitionaire and @stepancar, closes #494 and #768) ### “Fork me” ribbon One more step to make Styleguidist usable for documenting open source projects: ![](https://d3vv6lp55qjaqc.cloudfront.net/items/1t331n2a3v0F2i290K0Z/Image%202018-03-22%20at%209.13.11%20AM.png) New config option to enable the ribbon, define the URL and change the label: ```js module.exports = { ribbon: { url: 'http://example.com/', text: 'Fork me on GitHub' } }; ``` And two new [theme variables](https://github.com/styleguidist/react-styleguidist/blob/master/src/styles/theme.js) to change colors: `color.ribbonBackground` and `color.ribbonText`. (#861 by @glebez and @wkwiatek, part of #195, closes #647) ### Options to change CLI output on server start and build Two new options, `printServerInstructions` and `printBuildInstructions`: ```js module.exports = { printBuildInstructions(config) { console.log( `Style guide published to ${ config.styleguideDir }. Something else interesting.` ); } }; ``` (#878 by @roblevintennis, closes #876) ### Option to modify props A new option, `updateDocs` to modify props before rendering. For example, you can load a component version number from a JSON file: ```js module.exports = { updateDocs(docs) { if (docs.doclets.version) { const versionFilePath = path.resolve( path.dirname(file), docs.doclets.version ); const version = require(versionFilePath).version; docs.doclets.version = version; docs.tags.version[0].description = version; } return docs; } }; ``` (#868 by @ryanoglesby08) ### Limited support for named exports Styleguidist used to require default exports for components. Now you can use named exports too, though Styleguidist still supports only one component per file. I hope this change will make some users happier, see more details [in the docs](https://react-styleguidist.js.org/docs/components.html#loading-and-exposing-components). (#825 by @marcdavi-es, closes #820 and #633) ### Allow arrays of component paths in sections More flexibility in structuring your style guide: ```js module.exports = { sections: [ { name: 'Forms', components: [ 'lib/components/ui/Button.js', 'lib/components/ui/Input.js' ] } ] }; ``` (#794 by @glebez, closes #774) ### Sort properties by `required` and `name` attributes This change should make props tables more predictable for the users. Instead of relying on developers to sort props in a meaningful way, Styleguidist will show required props first, and sort props in each group alphabetically. To disable sorting, use the identity function: ```javascript module.exports = { sortProps: props => props }; ``` (#784 by @dotcs) ## Bug fixes * Allow `Immutable` state in examples (#870 by @nicoffee, closes #864) * Change template container ID to prevent clashes (#859 by @glebez, closes #753) * Better props definitions with Flow types (#781 by @nanot1m)

Sort properties by 'required' and 'name' attributes

182fc89

sapegin requested changes Jan 22, 2018

View reviewed changes

dotcs added 2 commits January 23, 2018 09:15

Revert "Sort properties by 'required' and 'name' attributes"

34a925a

This reverts commit 182fc89.

Re-implement sorting of properties in props-loader

3cf04d4

dotcs commented Jan 23, 2018

View reviewed changes

sapegin requested changes Jan 24, 2018

View reviewed changes

dotcs added 3 commits January 24, 2018 11:25

Disable react/prop-types rule in file Props.spec.js

eb7587a

Isolate sortProps method, add tests

5bf7a86

Add JSDoc for sortProps method

60d94ad

Introduce propsTransform config function

43b18f4

sapegin requested changes Jan 26, 2018

View reviewed changes

Improved docs, added more tests for demonstrating how sorting could b…

3624247

…e configured

sapegin reviewed Jan 26, 2018

View reviewed changes

docs/Configuration.md Outdated

```javascript

module.exports = {

transformProps: function(props) {

Copy link

Member

sapegin Jan 26, 2018

Choose a reason for hiding this comment

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

props => props ;-)

sapegin added this to the 6.3.0 milestone Jan 26, 2018

Update example code (use arrow function)

45753c6

sapegin approved these changes Jan 26, 2018

View reviewed changes

sapegin and others added 2 commits March 20, 2018 15:03

Merge branch 'master' into feature/sort-props

b46f35b

Fix tests

719126b

sapegin merged commit c572185 into styleguidist:master Mar 20, 2018

		@@ -12,6 +12,18 @@ const consts = require('../scripts/consts');

		const ERROR_MISSING_DEFINITION = 'No suitable component definition found.';

		function sortProps(props) {


		Type: `Function`, optional

		Function that transforms component properties.

Sort properties by 'required' and 'name' attributes #784

Sort properties by 'required' and 'name' attributes #784

Conversation

dotcs commented Jan 22, 2018

codecov-io commented Jan 22, 2018 • edited Loading

Codecov Report

sapegin left a comment

Choose a reason for hiding this comment

dotcs commented Jan 22, 2018

sapegin commented Jan 22, 2018

Choose a reason for hiding this comment

sapegin Jan 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dotcs commented Jan 23, 2018

dotcs commented Jan 24, 2018

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

dotcs commented Jan 24, 2018

n1313 commented Jan 25, 2018 • edited Loading

dotcs commented Jan 25, 2018

sapegin commented Jan 25, 2018

dotcs commented Jan 25, 2018

n1313 commented Jan 25, 2018

sapegin commented Jan 25, 2018

dotcs commented Jan 25, 2018

kof commented Jan 25, 2018

n1313 commented Jan 25, 2018

kof commented Jan 25, 2018

kof commented Jan 25, 2018

n1313 commented Jan 25, 2018

dotcs commented Jan 26, 2018 • edited Loading

sapegin commented Jan 26, 2018

dotcs commented Jan 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

n1313 commented Jan 26, 2018

sapegin commented Jan 26, 2018

Choose a reason for hiding this comment

okonet commented Jan 26, 2018

sapegin commented Mar 20, 2018

codecov-io commented Jan 22, 2018 •

edited

Loading

sapegin Jan 23, 2018 •

edited

Loading

n1313 commented Jan 25, 2018 •

edited

Loading

dotcs commented Jan 26, 2018 •

edited

Loading