Add method groups sorting #41
Conversation
| name, | ||
| value, | ||
| options = { | ||
| domain: window.location.hostname.split('.').slice(-2).join('.'), |
There was a problem hiding this comment.
Maybe it is better to specify an empty object as a default value of options, declare somewhere earlier defaultOptions = { domain: .... } , and then merge options from params and default options. What do you think?
With current defaults declaration we must duplicate domain option where we call cookies.set, otherwise it will be overwritten
There was a problem hiding this comment.
You are right. Fixed.
| @@ -41,11 +45,16 @@ class MainLayout extends React.PureComponent { | |||
|
|
|||
| this.warnings = props.topLevelMeta.warnings; | |||
|
|
|||
| this.isGroupsSortingEnabled = cookies.get({ | |||
| name: SORT_METHOD_GROUPS_COOKIE_NAME, | |||
| }) === true.toString(); | |||
There was a problem hiding this comment.
I can't say that this is totally wrong, I see why you need this (I mean true.toString()). But I have strange feeling that it looks a bit awkward.
If you later choose another value for cookie (so it's not 'true' or 'false'), you need to change it here too or it will break down. I would prefer something like cookie is set / cookie is undefined to use good old double negation !!.
But as I said, this is just my feeling, you are free to stay with this implementation
There was a problem hiding this comment.
Thank you! I have fixed it
Solovev-A
force-pushed
the
improvement-method-groups-order
branch
from
2d27069
to
667f345
Compare
Solovev-A
force-pushed
the
improvement-method-groups-order
branch
from
667f345
to
03be83d
Compare
|
Updated sorting for organizing |
Solovev-A
changed the title
There was a problem hiding this comment.
It looks great! @NetDead, сould you merge this pull request?
This PR adds method groups sorting option.
sort.mp4