Skip to content
This repository was archived by the owner on Sep 19, 2024. It is now read-only.

Conversation

klauste
Copy link
Contributor

@klauste klauste commented Sep 4, 2020

LRN-28950

null,
false
'learnosity-items',
$lrn_items_api_url,

Choose a reason for hiding this comment

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

I think the indentation change here was checked in by accident? It looked correct before here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indentation before was partially not with spaces, fixed it in new commit

null,
false
);
wp_enqueue_script(

Choose a reason for hiding this comment

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

Indentation of this function call is wrong, so would be good to fix here thanks

$lrn_items_api_url,
array(),
null,
true

Choose a reason for hiding this comment

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

Did you mean to change this in_footer variable to true? If so, how come?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, adding comment:
/*
* For items and author, in_footer is set to true. This is on purpose as the information, whether
* the read direction is right to left is set in the plugin short-code. If we loaded the scripts
* in the header, they'd be loaded before the short-code scripts are encountered, thus we
* wouldn't know whether to add the rtl flag in the src tag.
*/

true
);

// Before the script tags are added to page, add the rtl data attribute

Choose a reason for hiding this comment

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

Indentation wrong here, too

}

/**
* Adds data-lrn-dir="rtl" to the source tag if $this->>is_rtl is true

Choose a reason for hiding this comment

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

Typo ->> -> ->

public function render_items($attrs, $content)
{
$items_embed = new ItemsEmbed($attrs, 'inline', $content);
if (isset($attrs['rtl']) && $attrs['rtl'] === 'true') {

Choose a reason for hiding this comment

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

Logic is repeated 4 times, ideally extract to function

@klauste klauste force-pushed the LRN-28950/FEATURE/support-rtl-plugin branch from a29a689 to caaa519 Compare September 11, 2020 09:51
@klauste klauste merged commit 2a7bfab into master Sep 11, 2020
@klauste klauste deleted the LRN-28950/FEATURE/support-rtl-plugin branch September 11, 2020 09:51
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