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

Add ef_week_first_day as script data to prevent echoing script tags #582

Merged
merged 2 commits into from
Jan 24, 2020

Conversation

ryelle
Copy link
Member

@ryelle ryelle commented Jan 22, 2020

This attaches the var definition to the script itself, so it will only output when the script tag is actually printed, rather than just enqueued.

If you only call wp_enqueue_scripts, this just registers & enqueues the scripts, but doesn't necessarily print them. This was causing a conflict when the PWA plugin is active. It calls admin_enqueue_scripts to set up scripts for caching, but it never outputs the script. See WordPress/wordcamp.org#328 for my (hopefully temporary 😁) workaround.

Steps to Test

  1. Check out PR.
  2. View source on a page, see that the variable is defined immediately before the script tag now
  3. Check that the calendar still renders correctly

To be honest I don't know if more testing is necessary, but that's what I did ^

This attaches the `var` definition to the script itself, so it will only output when the script tag is actually printed, rather than just enqueued.
wp_enqueue_script( 'jquery-ui-datepicker' );

//Timepicker needs to come after jquery-ui-datepicker and jquery
wp_enqueue_script( 'edit_flow-timepicker', EDIT_FLOW_URL . 'common/js/jquery-ui-timepicker-addon.js', array( 'jquery', 'jquery-ui-datepicker' ), EDIT_FLOW_VERSION, true );
wp_enqueue_script( 'edit_flow-date_picker', EDIT_FLOW_URL . 'common/js/ef_date.js', array( 'jquery', 'jquery-ui-datepicker', 'edit_flow-timepicker' ), EDIT_FLOW_VERSION, true );
wp_add_inline_script( 'edit_flow-date_picker', sprintf( 'var ef_week_first_day = \'%s\';', esc_attr( get_option( 'start_of_week' ) ) ), 'before' );
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to use wp_json_encode here? I've got a similar PR which I based off a suggestion from some VIP documentation

Other than that, tested it and it looks good to go!

Copy link
Member Author

Choose a reason for hiding this comment

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

Done! TIL that function's not just for objects 🙂

Copy link
Contributor

@cojennin cojennin left a comment

Choose a reason for hiding this comment

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

Looks good!

@cojennin cojennin merged commit 82eac6b into master Jan 24, 2020
@cojennin cojennin deleted the fix/calendar-script-tag branch April 11, 2020 14:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants