Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for mobile app #122

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

andrewhancox
Copy link

Includes:
Enhancement to allow embedding courses from a specific category
e.g. {coursecards|_categoryID}

Elimination of deprecated function call

Change to allow use in mobile app:
The mobile app does not handle background images set by in-line styles, they work the first time you view the content and then fail when you navigate away and back again.

@michael-milette
Copy link
Owner

michael-milette commented Sep 30, 2020

Hi @andrewhancox ,

Thank you for your contribution. I can see how being able to specify the category ID will be very useful.

With regards to the syntax, I would prefer if you would use a space to delimit the command and the category ID instead of an underscore. This is in keeping with the format of the rest of the FilterTags. In addition, one of my future plans was to be able to list courses with a particular tag. For example, if I wanted to present a list of "Featured" courses on the FrontPage, I might want to have something like {coursecards tag=featured} or {coursecardstag featured}. In fact, it might simplify your code if you just made a {coursecardscategory XX} where XX is the ID of the category. I welcome your thoughts on this.

With regards to the deprecated function, could you tell me which function it is that you are referring to? As far as I can tell, they all seem to still be current from Moodle 3.2 through 3.9. I am happy to accept the change but I just need a good reason as the replacement functions you are proposing are also valid from 3.2 to 3.9. Otherwise it just feels like changing for the purpose of change.

I noticed you moved all of the CSS into the styles.css file. Normally I would agree with you however my plan was to actually make the height and width of the cards configurable in the plugin settings which is why they were left in the PHP. Any alternative suggestions? I am definitely in favour of enabling support in the mobile app whenever possible.

Best regards,

Michael

@michael-milette michael-milette added the feature request New features, enhancements label Sep 30, 2020
@andrewhancox
Copy link
Author

The deprecated function is file_encode_url - The deprecation notice is here: MDL-31071

The main reason I moved the styling to a css file was so it would be ignored by the app, some of these directives could definitely be moved back as in-line styles to make the configurable. I can't quite remember exactly which ones were breaking the app.

I'll make the suggested change to the syntax. I would suggest not introducing a new substitution as it would introduce duplicated code.

@michael-milette
Copy link
Owner

michael-milette commented Sep 30, 2020

I had noticed MDL-31071. However the ticket was created 8 years ago and is still open - not even in progress. There are still MANY instances of file_encode_url() scattered throughout Moodle 3.9. Will it ever actually be deprecated? I don't know... maybe 4.0? There doesn't seem to be any rush. That said, it does work and your code is just as compatible as file_encode_url (back to Moodle 3.2 as far as I can tell. So I will agree. That way, should it ever end up deprecate, the tag will still work (develop for the future, right?) :-)

@michael-milette michael-milette added the hacktoberfest Contributions qualify for Digital Ocean Hacktoberfest label Oct 5, 2020
Copy link
Owner

@michael-milette michael-milette left a comment

Choose a reason for hiding this comment

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

With regards to the syntax, I would prefer if you would use a space to delimit the command and the category ID instead of an underscore. This is in keeping with the format of the rest of the FilterCodes. So the tag format should be {coursecards id=xx,yy,zz} where xx,yy,zz would be one or more category IDs.

One of my future plans will be to add the ability to list courses with a particular tag. For example, I might want to present a list of courses that have been tagged with the word "featured" on the FrontPage so the syntax might be something like {coursecards tag=featured}. This is just for your information. I don't expect you create that functionality just yet as part of your PR (though you are welcome to do it).

As for the CSS, my plan was to actually make the height and width of the cards configurable in the plugin settings which is why these were left in the PHP.

If you would please be so kind as to make these changes, I would really appreciate it.

Copy link
Owner

@michael-milette michael-milette left a comment

Choose a reason for hiding this comment

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

As for the CSS, my plan was to actually make the height and width of the cards configurable in the plugin settings which is why these were left in the PHP.

Copy link
Owner

@michael-milette michael-milette left a comment

Choose a reason for hiding this comment

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

Forgot to mention, please don't change the version.php file just yet. I only change the version number in that file with official releases so that people can test new functionality and revert back without having to re-install the plugin.

Copy link
Owner

@michael-milette michael-milette left a comment

Choose a reason for hiding this comment

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

The change from file_encode_url to make_pluginfile_url is acceptable. Thank you.

@michael-milette
Copy link
Owner

Hi @andrewhancox , I was just wondering if you had submitted the changes yet? I am not trying to rush you or anything however, I also don't want you to think I have forgotten you if you did submit a new pull request but I didn't receive it.

Best regards,

Michael

@andrewhancox
Copy link
Author

I've made a new commit to the branch. I've altered the syntax of the placeholder as requested and re-instated the in-line styles related to sizing.
One issue I bumped up against in testing this was pre tags being injected by the editor un-expectedly. This caused the layout to go nuts... is it worth flagging this in the docs?

@michael-milette michael-milette added the hacktoberfest-approved Approved for Digital Ocean Hacktoberfest label Nov 7, 2020
@michael-milette michael-milette force-pushed the master branch 5 times, most recently from 93973ec to be79b30 Compare November 23, 2020 09:06
…ted function call (file_encode_url), make compatible with mobile app (no support for background images on in-line styles).
@andrewhancox
Copy link
Author

I've just realised I never got this closed off. I've rebased and done a force push - I don't think there were any requested changes outstanding though...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request New features, enhancements hacktoberfest Contributions qualify for Digital Ocean Hacktoberfest hacktoberfest-approved Approved for Digital Ocean Hacktoberfest
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants