-
-
Notifications
You must be signed in to change notification settings - Fork 610
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
blade conversion #2602
blade conversion #2602
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job. A few comments (most of them are the same thing just trying to find all locations).
@@ -25,10 +25,11 @@ | |||
<script> | |||
jQuery(document).ready(function(){ | |||
|
|||
@if (isset($_GET['closeModal'])) | |||
<?php if (isset($_GET['closeModal'])): ?> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not @if
? This should be blade
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was getting syntax error in the editor, while using @if
inside the script tag, might be vs setting issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
solved
<small class="align-center">{{ $disclaimer }}</small> | ||
@endif | ||
|
||
{!! $tpl->viewFactory->make($tpl->getTemplatePath('canvas', 'modals'), $__data)->render() !!} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is required anymore.
}); | ||
} | ||
|
||
leantime.<?= $canvasName ?>CanvasController.setRowHeights(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace with blade syntax
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or I guess only "goal" since this should be replaced anyways
<?php } ?> | ||
|
||
|
||
<?php if (isset($_GET['showModal'])) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
update to use blade syntax
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i implemented the changes
@@ -25,10 +25,11 @@ | |||
<script> | |||
jQuery(document).ready(function(){ | |||
|
|||
@if (isset($_GET['closeModal'])) | |||
<?php if (isset($_GET['closeModal'])): ?> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
solved
window.onloa39d = function() { | ||
if (!window.jQuery) { | ||
//It's not a modal | ||
location.href = "<?= BASE_URL ?>/<?= $canvasName ?>canvas/showCanvas?showModal=<?= $canvasItem['id']; ?>"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replaace with "goal" in url
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also replace <?= with blade syntax
window.onload = function() { | ||
if (!window.jQuery) { | ||
//It's not a modal | ||
location.href = "<?= BASE_URL ?>/goalcanvas/showCanvas?showModal=<?php echo $canvasItem['id']; ?>"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please replace php with blade
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
variable name update
Link to ticket
Please add a link to the GitHub issue being addressed by this change.
Description
Please include a short description of the suggested change and the reasoning behind the approach you have chosen.
Screenshot of the result
If your change affects the user interface, you should include a screenshot of the result with the pull request.
Checklist
If your code does not pass the requirements on the checklist, you should add a comment explaining why this change
should be exempt from the list.
Additional comments or questions
If you have any further comments or questions for the reviewer, please add them here.