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.

Sign up for GitHub

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

Jump to bottom

feat: canonically provide $ACT to javascript #2190

Merged
merged 7 commits into from Apr 5, 2018
Merged

feat: canonically provide $ACT to javascript #2190

merged 7 commits into from Apr 5, 2018

Conversation

micgro42
Copy link
Collaborator

@micgro42 micgro42 commented Dec 11, 2017 •
edited

It would be helpful if there were a reliable way to determine the current mode in javascript, for example, if we only want to execute some javascript in show or preview modes. This writes the final mode to the $JSINFO object right before it is written out as a header, so we can hopefully capture the actual mode without a plugin changing it later on.

To Do/Discuss

  • I would like to initialize $JSINFO in lib/exe/detail.php as well. Which keys should be included?
  • There are several lines in lib/exe/js.php that might be suitable to be included in JSINFO (see below). Which one should we include in JSINFO and can we somehow show a deprecation notice if the old variables are used?

https://github.com/splitbrain/dokuwiki/blob/479b8450994e5a47eff744c623b3ae14afcee19c/lib/exe/js.php#L94-L103

@micgro42
feat: canonically provide $ACT to javascript
479b845 
It would be helpful if there were a reliable way to determine the
current mode in javascript, for example if we only want to execute some
javascript in `show` or `preview` modes. This writes the final mode to
the JSINFO object right before it is written out as header, so we can
hopefully capture the actual mode without an plugin changing it later
on.
@micgro42
feat: DOKU_UH[N|C] to jsinfo and add deprecation message
034db88 
I'm not sure that setting the constants in template.php is the best
place. However if we set them in doku.php, we have to duplicate it in
lib/exe/mediamanager.php and lib/exe/detail.php, which is not ideal as
well.
@micgro42
Copy link
Collaborator Author

I have now added DOKU_UHN and DOKU_UHC to JSINFO. However, I'm not sure if setting these in template.php is the best place. On the other hand, if we set them in doku.php, then we have to duplicate that code in lib/exe/mediamanger.php and lib/exe/detail.php as well. That would not be ideal either.

Thoughts?

@Klap-in
Copy link
Collaborator

Klap-in commented Dec 11, 2017 •
edited

The shortcuts UHN and UHC are existing ones? I would prefer names that are more self-explaining...
e.g. DOKU_useHeadingNav, DOKU_useHeadingCont?

edit: better example:
JSINFO.useHeadingNav, JSINFO.useHeadingCont
or even JSINFO.useHeadingNavigation, JSINFO.useHeadingContent

@Klap-in
Copy link
Collaborator

Klap-in commented Dec 11, 2017 •
edited

variable JSINFO is now set in tpl_metaheaders(), which is called by lib/tpl/<name>/main.php, lib/tpl/<tplname>/detail.php and lib/tpl/<tplname>/mediamanager.php. These files are called at the end of respectively doku.php, lib/exe/detail.php and lib/exe/mediamanager.php.

For including the JSINFO in the head as <script> element this is the last moment, so far as I see.

In doku.php and lib/exe/mediamanager.php the JSINFO is filled with id and namespace entries.
For lib/exe/detail.php these entries are not set, but it is defined.

Why do you consider moving it from tpl_metaheaders() to doku.php?

@micgro42
Copy link
Collaborator Author

I renamed the fields.

I don't want to move the place where the javascript variable JSINFO is set, but I'm not sure if filling the PHP global variable $JSINFO in tpl_metaheaders is the best place to do so.

splitbrain
splitbrain reviewed Apr 5, 2018
View reviewed changes
Copy link
Collaborator

@splitbrain splitbrain left a comment

Choose a reason for hiding this comment

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

except for location/naming this looks fine to me

inc/template.php Outdated
@@ -352,6 +351,20 @@ function tpl_metaheaders($alt = true) {
return true;
}

function _tpl_ensureJSINFO() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure if this should be in inc/template.php and I'm also not so keen about the name. Maybe initJSInfo or upgradeJSInfo?

@splitbrain splitbrain added this to the 🐱 Greebo milestone Apr 5, 2018
@micgro42
refactor: rename and move function to set $JSINFO
0c39d46 
This is a more consistent place relative to pageinfo()
@splitbrain splitbrain merged commit d9e82b0 into master Apr 5, 2018
@splitbrain splitbrain deleted the betterJSINFO branch April 5, 2018 08:31
@micgro42 micgro42 mentioned this pull request Apr 6, 2018
Closed
4 tasks
@Klap-in
Copy link
Collaborator

Klap-in commented Jun 26, 2018

DOKU_UHC is still in use in dokuwiki js: https://github.com/splitbrain/dokuwiki/blob/65a6bb072adbc4fdbfd45391b93c089e18c997ac/lib/scripts/linkwiz.js#L230

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

@splitbrain splitbrain splitbrain left review comments

Assignees
No one assigned
Labels
Needs Feedback/Discussion ToDo: needDocUpdate
Projects
None yet
Milestone
🐱 Greebo
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@micgro42 @Klap-in @splitbrain