Skip to content

Conversation

@timhunt
Copy link
Contributor

@timhunt timhunt commented Jan 20, 2022

This was breaking RTL in the OU theme. This fix works there. Because of issue #3, I cannot easily add a unit test (but I would if the tests passed).

Anyway, please could this be merged (along with the other pull request that has been sitting around for months.

Then, please, could this library be updated in Moodle core - these are bug fixes, so should be done on all stable branches.

@timhunt
Copy link
Contributor Author

timhunt commented Jan 20, 2022

By the way Diff easier to read if you ignore space changes.

@timhunt
Copy link
Contributor Author

timhunt commented Jan 20, 2022

For reference, before this fix, the error message was

Exception - Call to undefined method Sabberworm\CSS\Value\Size::getListComponents()

Stack trace:

    line 273 of \lib\rtlcss\RTLCSS.php: Error thrown
    line 223 of \lib\rtlcss\RTLCSS.php: call to MoodleHQ\RTLCSS\RTLCSS->processRule()
    line 55 of \lib\classes\rtlcss.php: call to MoodleHQ\RTLCSS\RTLCSS->processDeclaration()
    line 192 of \lib\rtlcss\RTLCSS.php: call to core_rtlcss->processDeclaration()
    line 63 of \lib\rtlcss\RTLCSS.php: call to MoodleHQ\RTLCSS\RTLCSS->processBlock()
    line 1859 of \lib\outputlib.php: call to MoodleHQ\RTLCSS\RTLCSS->flip()
    line 1837 of \lib\outputlib.php: call to theme_config->rtlize()
    line 1161 of \lib\outputlib.php: call to theme_config->post_process()
    line 205 of \theme\styles.php: call to theme_config->get_css_content()
    line 171 of \theme\styles.php: call to theme_styles_generate_and_store()

@timhunt
Copy link
Contributor Author

timhunt commented Jan 20, 2022

The required unit tests probably look something like timhunt@2673897, if anyone can get the tests working.

@timhunt
Copy link
Contributor Author

timhunt commented Jan 21, 2022

Amended commit pushed. (Also rebased)

Copy link
Member

@sarjona sarjona left a comment

Choose a reason for hiding this comment

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

Thanks Tim! Now the patch looks good and it's working as expected so I'm more than happy to merge it! :-)

@sarjona sarjona merged commit f4d61fe into moodlehq:master Jan 21, 2022
@timhunt
Copy link
Contributor Author

timhunt commented Jan 21, 2022

Awesome. Thanks :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants