Add link to selected default-snippet in SnippetAreas view #5392
Conversation
niklasnatter
force-pushed
the
feature/default-snippet-link
branch
from
826636d
to
26268ec
Compare
|
For me this should target 2.1 branch as a bugfix, as it was just forgot there :) |
|
I did test it and it did work as expected but it would be great if @danrot could have a look at the code also as he is most familiar with the deep links. |
| @@ -1,7 +1,8 @@ | |||
| // @flow | |||
| import React from 'react'; | |||
| import {shallow, mount, render} from 'enzyme'; | |||
| import {Router} from 'sulu-admin-bundle/services'; | |||
| import Router from 'sulu-admin-bundle/services/Router'; | |||
| import Route from 'sulu-admin-bundle/services/Router/Route'; | |||
There was a problem hiding this comment.
Shouldn't import {Route, Router} from 'sulu-admin-bundle/services'; also work?
| <Button | ||
| className={snippetAreasStyles.titleButton} | ||
| onClick={this.handleSnippetClick} | ||
| skin="link" |
There was a problem hiding this comment.
I don't think that this results in the desired design, since the other deep links are also shown differently (i.e. without an underline). The other deep links are simply div elements with a button role, would keep it the same way here for consistency.
There was a problem hiding this comment.
the reason i implemented this using a Button is that the react/jsx-no-bind eslint rule forces me to extract this "simple <div> into a separate component.
this is the second time in a short timespan that this rule complicates the implementation of a simple component. i would really like to enable the allowArrowFunctions of it.
There was a problem hiding this comment.
Personally I don't think that this makes it a lot more complicated, and extracting separate components is most of the time not such a bad idea. Also, as described in the rule documentation, that should speed up rendering. I tend to believe that, unless I see a benchmark proofing the opposite. I think we already have talked about this, and this rule would indeed not work anymore with hooks, but since we are not using them yet I would keep this rule for now.
There was a problem hiding this comment.
do you think its a good idea to extract a component in this case?
regarding performance, i tend to dont believe that, unless I see a benchmark proofing the opposite 馃槈
see https://cdb.reacttraining.com/react-inline-functions-and-performance-bdff784f5578
There was a problem hiding this comment.
TBH, although it is an unpopular opinion, I would do it, because MobX advises to use small components (which is MobX specific, so that's for sure not mentioned in that article) and because the rule also avoids code like this:
<div
onClick={() =>{
methodCall1();
methodCall2();
if (someValue == 5) {
methodCall3();
} else {
methodCall4();
methodCall5();
}
}}
/>This code should IMO definitely be extracted into a separate function, and the rule also enforces this.
These two reasons alone would be enough for me to accept the minor inconvenience creating a separate component causes. But if we still decide to not do that anymore I would change that in the eslint config instead of disabling it using comments, because it has about the same effect (I do not want to think or argue at every PR if that is a valid edge case for disabling this rule).
There was a problem hiding this comment.
After some thinking this discussion took maybe a bit an unnecessary turn... I didn't dislike the Button solution very much, the only real concern for me was that the styling did not match. But this could be solved by introducing another skin (maybe "text"?) that would display it in the way we want it to be displayed. This way we could keep the rule, and the issue you have mentioned would be solved in many cases. What do you think?
There was a problem hiding this comment.
Yes, a text skin for the Button component sounds okay to me. It will also prevent the issues with the react/jsx-no-bind for the most simple cases in the future, so thats nice 馃檪
| const router = new Router({}); | ||
| router.attributes = { | ||
| webspace: 'sulu', | ||
| }; |
There was a problem hiding this comment.
Why are the attributes removed in all these tests? Haven't they been necessary before as well?
There was a problem hiding this comment.
the sulu-admin-bundle/services/Router is mocked to check the calls to the navigate method. the attributes are already set in the mock: https://github.com/sulu/sulu/pull/5392/files#diff-1d96f77c6059525b08c9dfc4dabee33aR22-R27
niklasnatter
force-pushed
the
feature/default-snippet-link
branch
from
26268ec
to
15cbc63
Compare
niklasnatter
force-pushed
the
feature/default-snippet-link
branch
from
15cbc63
to
4c76c2d
Compare
What's in this PR?
This PR adjusts the
SnippetAreasview to provide a link to to the selected snippet. TheSnippetAreasview is used to set default snippets for a webspace.Why?
Because the
Selection,SingleSelection,SmartContentandTeaserSelectionprovide a similar link feature. Also, it is quite annoying to search for the name of the selected snippet in the snippet list if there are a lot of snippets 馃檪