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

Feature/gh 2307 #2310

Merged
merged 25 commits into from May 6, 2019
Merged

Feature/gh 2307 #2310

merged 25 commits into from May 6, 2019

Conversation

clairenoelle
Copy link
Contributor

@clairenoelle clairenoelle commented Mar 23, 2019

Move /images under /public and replace references with $GLOBALS variable

below added by bradymiller:
Up For Grabs demo for this PR is at : https://www.open-emr.org/wiki/index.php/Development_Demo#Zeta_-_Up_For_Grabs_Demo

@bradymiller
Copy link
Sponsor Member

hi @cmccrea23 , Just completed a code review. Looking forward to the next revision! -brady

@bradymiller
Copy link
Sponsor Member

bradymiller commented Mar 28, 2019

hi @cmccrea23 ,

Turns out I was able to remove all of library/css/images in addition to the query-ui-1.8.21.custom.css script after some code mods. See here for the commit that did this, which is now in the codebase:
0ed27f1

So, recommend reverting your most recent commit in your branch (and PR) to avoid bunch of merge conflicts in future:

git revert dd624eca4df4eae0a4da05e10e0c87888b9a8b86

@clairenoelle
Copy link
Contributor Author

clairenoelle commented Mar 28, 2019

hi @cmccrea23 ,

Turns out I was able to remove all of library/css/images in addition to the query-ui-1.8.21.custom.css script after some code mods. See here for the commit that did this, which is now in the codebase:
0ed27f1

So, recommend reverting your most recent commit in your branch (and PR) to avoid bunch of merge conflicts in future:

git revert dd624eca4df4eae0a4da05e10e0c87888b9a8b86

Awesome! 👏 I've changed the $GLOBALS['webroot'] back for the relevant references in download_qrda.php. Also found that the downloadAllXML() function I found in line 153 still doesn't exist, but there is an active downloadXML(). Could that function be what was meant in line 153?

@bradymiller
Copy link
Sponsor Member

hi @cmccrea23 , Review complete. Getting close. After above is addressed in your next revision, will then plan testing.

@bradymiller
Copy link
Sponsor Member

Forgot to add. Can also remove the following 2 images:
check.png
loading.gif

@clairenoelle
Copy link
Contributor Author

I think I used the $GLOBAL variable correctly here, but I've never worked with PHP before so I'm not certain. Would you mind taking another look? Same with setting the path in the TreeMenu, I'm not confident I did it correctly. Sorry in advance, and thanks for your help 🙏

library/edi.inc Outdated Show resolved Hide resolved
@bradymiller
Copy link
Sponsor Member

hi @cmccrea23 ,
Getting very close. Left more instructions above. I'll show how to make this work in the smarty template script (item with TODO in my post) tomorrow.

@clairenoelle
Copy link
Contributor Author

Sorry this took so long for me to do. I just started a new job at the beginning of the month and haven't had much time for outside projects 😣hope this fixes it!

@bradymiller
Copy link
Sponsor Member

hi @cmccrea23 , no prob on the delay and congrats on the job! tortoise always wins the race :)
I only noted one more minor issue on code review. When that's addressed, then I'll do testing and last review before bringing it into the codebase. thanks! -brady

@bradymiller
Copy link
Sponsor Member

bradymiller commented May 6, 2019

hi @cmccrea23 ,
Very nice work on this large project!
Did testing and made some fixes, which brought into your PR:
1fab949
Going to bring your PR into the main codebase now. Thanks for the code contribution!
-brady

@bradymiller bradymiller merged commit e9488ce into openemr:master May 6, 2019
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

2 participants