Support for print stylesheets #16857
Conversation
|
A new inspection was created. |
| @@ -155,7 +155,11 @@ public function __construct( $renderAs, $appId = '' ) { | |||
| $web = $info[1]; | |||
| $file = $info[2]; | |||
|
|
|||
| $this->append( 'cssfiles', $web.'/'.$file . '?v=' . self::$versionHash); | |||
| if (substr($file, -strlen('print.css')) === 'print.css') { | |||
There was a problem hiding this comment.
This should be documented in the app developer documentation
There was a problem hiding this comment.
I'll add this to the css documentation as soon as this is merged.
|
Looks good to me |
|
@DeepDiver1975 feature freeze ? |
you are for sure right - thx for the hint |
|
Wouldn't it be easier to use the |
|
Until now every stylesheet was limited to screen, because You can choose whatever fits best for your use case. |
|
What's the next step ? @jancborchardt can you review ? |
|
@Henni so this is good to go from your view? |
|
@jancborchardt yes it is |
|
@Henni are the failures related then? Because it looks good for me otherwise. |
|
👍 from me. A JS code review please @MorrisJobke @PVince81 @ChristophWurst |
|
@jancborchardt the errors shouldn't be related. |
|
@owncloud-bot retest this please |
Also, can’t be merged cause of failing tests @DeepDiver1975 @Henni I guess this has to wait for 9.0 |
a679c03 to
d9c782c
Compare
|
@PVince81 are all your concerns addressed? THX |
|
Yes. Has someone tested this with and without assets pipeline ? (I can see changes related to that) |
|
Looks like this breaks when assets pipeline is enabled.
This works properly on master (854f9db) |
|
I'll have a look |
d9c782c to
0825165
Compare
|
I fixed the issue with the asset pipeline. |
|
This does not touch tests, does it mean they still pass as they are, or are there no tests? |
|
@nickvergessen good question. I didn't look for tests regarding this. |
|
there are none 🙊 |
0825165 to
99b9ec4
Compare
|
rebased |
|
@Henni are all issues sorted out? THX |
|
Moving to 9.1 since we’re past feature freeze and this is not critical. |
|
This breaks some usages, will come up with a follow up PR |
|
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
I removed the restriction
media="screen"for all stylesheets and added the restrictionmedia="print"to all stylesheets with the nameprint.css.Maybe the restriction
media="screen"could be added to all stylesheets with the namescreen.cssand similar forprojection.cssThis prepares the core for owncloud/tasks#119 and owncloud/mail#717.
Feedback is especially welcome for environments with the AssetPipeline enabled.