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

Hook for page action buttons #6090

Merged
merged 5 commits into from
Jan 12, 2023

Conversation

robertdown
Copy link
Member

@robertdown robertdown commented Jan 11, 2023

Create a new event and interface for injecting buttons into the page header rendered by OemrUI. Convert the current help icon, action icon, and expand icons into listeners to the new event. Move the pageHeading() rendering to Twig.

TBD

1. The Action Icon has some logic I need to still resolve
2. Create spot to inject custom Javascript to actually do something with your fancy new button.

Create a new event and interface for injecting buttons into the page header rendered by OemrUI. Convert the current help icon, action icon, and expand icons into listeners to the new event
@robertdown robertdown marked this pull request as draft January 11, 2023 23:44
@robertdown robertdown self-assigned this Jan 12, 2023
The buttons actually work now!
@robertdown robertdown marked this pull request as ready for review January 12, 2023 05:35
src/OeUI/OemrUI.php Outdated Show resolved Hide resolved
@bradymiller
Copy link
Sponsor Member

@robertdown , just noted some escaping stuff. Otherwise code looks great to me :) (btw, I didn't test it)

@robertdown
Copy link
Member Author

Updated the way javascript interacts with the interface

The interface dictates getAttributes() must return an array of attributes to be injected into the element. This means you could do the following:

public function getAttributes()
{
    return [
        "onclick" => "function(){ alert('do something'); }"
    ];
}

which would render the onclick attribute (Though the escaping would be problematic because it gets run through attr not a js-specific escape.

OR, you could leverage getClickHandlerTemplateName() and getClickHandlerFunctionName() which returns the name of the twig template containing your function and the name of the function itself. If you return those, twig will automatically add an event listener to your button.

Still some downsides here, Ideally, the template being included should only contain 1 function, named whatever your pass in getClickHandlerTemplateName(), however, it could be expanded and is now effectively a hook for any javascript you want. Not sure how to force a better solution though.

@sjpadgett
Copy link
Sponsor Member

@robertdown Installing this PR errors javascript with
image
image

And

image

@robertdown
Copy link
Member Author

Hmmm, the patient finder page was one I tested against. Let me try to replicate

@sjpadgett
Copy link
Sponsor Member

Looks like a dependency scope issue. showActionClass looks like not making it into the DOM. You may need to change how it's declared.

@robertdown
Copy link
Member Author

@sjpadgett I could not replicate the error.

Steps I took

  1. docker-compose down -v to clear any lingering
  2. git checkout master
  3. git pull upstream master
  4. docker pull openemr/openemr:latest
  5. docker-compose up -d from development-easy to bring up a completely fresh instance

@sjpadgett
Copy link
Sponsor Member

Okay. I was treating it as a patch which should work and will still see what it takes to work that way. In the end I'll spin up a docker as you did.

@sjpadgett
Copy link
Sponsor Member

I think this has to do with the initial hide stats is in for the section and will account for why you're not seeing.
There is also the sql prepare error.
Or it could be my current dev environment so still checking it out

@robertdown
Copy link
Member Author

Not sure where the SQL error is coming from, I'm not doing any DB interaction. I built this off of master and I think Brady is planning an actual release, not a patch in the coming days

@sjpadgett
Copy link
Sponsor Member

Nope sorry that was me not updating sql. I have a simple fix for the javascript error which I'll add to my existing PR.
With that we should be good. give me a call if need to.

Comment on lines +544 to +545
elementIcon.classList.remove(showActionClass);
elementIcon.classList.add(hideActionClass);
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

elementIcon is never defined I can see. It is defined within the show_hide click() handler so it would be out of scope elsewhere.
I don't see how this would work in any case because while show_hide click will set as soon as routine is exited then you lose scope. elementIcon needs to be assigned/declared somewhere global.

@robertdown
Copy link
Member Author

Could you split it into its own PR. Still can't replicate so I want to test further

@sjpadgett
Copy link
Sponsor Member

If you're not seeing the error then you are not hitting the routine due to shouldDisplay not being set at around L-544 if (should Display) { ...}
After testing on this issue for awhile somehow something or flag was set that I no longer hit the errant code.
Easier if you give me a call to discuss. I can revert my fix if you want but the changes I made are necessary regardless if that section is never hit in our testing.

@robertdown
Copy link
Member Author

I can give you a call Sunday at the earliest. I trust whatever your fix is

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.

None yet

3 participants