Skip to content
This repository has been archived by the owner on Dec 2, 2022. It is now read-only.

442 theme_essential: added custom slide size proof-of-concept #444

Closed
wants to merge 1 commit into from

Conversation

kordan
Copy link

@kordan kordan commented Feb 9, 2015

This is my first pull request here in github. I apologise if it is not well done.
For sure I need to submit to your attention what follows:

  1. I had to add $THEME->sheets[] = 'essential'; as line 40 of config.php because my tags were not replaced. I wonder if I need to add $THEME->sheets[] = 'essential-rtl'; too. I tried but I get a mess. I am sure I need your supervision for this detail.
  2. I did not make any test with rtl languages.
  3. As far I can understand, the string $string['slideshowdesc'] is incorrect because it dials about "screen width < 767px = height 165px, width between 768px and 979px = height 225px and width > 980px = height 300px" but I found that the screen width are four.

@gjb2048
Copy link
Owner

gjb2048 commented Feb 9, 2015

Thanks for the patch.

Ok, adding $THEME->sheets[] = 'essential'; as line 40 of config.php because my tags were not replaced. I wonder if I need to add $THEME->sheets[] = 'essential-rtl'; is just plain wrong and will break the theme. If the selectors were not replaced then check them in browser tools to find out why as the 'essential.css' file is served before the custom CSS.

With '3' I'll check but I'm sure its correct as it deals with the three sets of media query bracketed resolutions already coded in the CSS.

With '2' if you did not test with RTL how did you know in '1' that you needed to add 'essential-rtl'?

Kind regards,

Gareth

@gjb2048
Copy link
Owner

gjb2048 commented Feb 9, 2015

Additional, the CSS changes need to go in the LESS files as they will be overwritten the next time 'grunt' is used. To learn about, this, please watch my screen casts for Shoehorn: https://moodle.org/mod/forum/discuss.php?d=279485 - the same mechanism applies to Essential.

@kordan
Copy link
Author

kordan commented Feb 9, 2015

With '2' if you did not test with RTL how did you know in '1' that you needed to add 'essential-rtl'?
By conceptually extending what I faced with essential.css

@gjb2048
Copy link
Owner

gjb2048 commented Feb 9, 2015

Still has to be tested though!

@gjb2048
Copy link
Owner

gjb2048 commented Feb 13, 2015

Duplicated by #445 - when updating, just use the same branch, rebase to combine commits and then do a forced push.

@gjb2048 gjb2048 closed this Feb 13, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants