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

SAK-50427 LTI Enhance the UI for Tool Deployment Feature #12851

Merged
merged 5 commits into from
Sep 5, 2024
Merged

SAK-50427 LTI Enhance the UI for Tool Deployment Feature #12851

merged 5 commits into from
Sep 5, 2024

Conversation

zhuoY121
Copy link
Contributor

@zhuoY121 zhuoY121 commented Sep 4, 2024

csev
csev approved these changes Sep 5, 2024
View reviewed changes
Copy link
Contributor

@csev csev left a comment

Choose a reason for hiding this comment

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

LGTM

@csev csev requested a review from ern September 5, 2024 13:04
@csev csev mentioned this pull request Sep 5, 2024
Closed
ern
ern reviewed Sep 5, 2024
View reviewed changes
basiclti/basiclti-tool/src/webapp/vm/lti_tool_edit.vm
</div>

<script>
//<![CDATA[
Copy link
Contributor

Choose a reason for hiding this comment

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

looks like you can remove this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Are you saying that the //<![CDATA[ is no longer needed for page validation?

ern
ern reviewed Sep 5, 2024
View reviewed changes
basiclti/basiclti-tool/src/webapp/vm/lti_tool_edit.vm

<form action="#toolForm("")" method="post" name="customizeForm" >
#if ($isEdit && $isAdmin && $autoStartUrl )
<div style="float:right" id="lti-advantage-div">
Copy link
Contributor

Choose a reason for hiding this comment

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

avoid inline style if possible?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think so - This is not a general thing - it is just one button that needs to float right. Any other approach would be harder to understand for future developers.

ern
ern reviewed Sep 5, 2024
View reviewed changes
basiclti/basiclti-tool/src/webapp/vm/lti_tool_edit.vm
</form>

#if (($isEdit && $isAdmin && $autoStartUrl) || $canDeploy)
<div id="modal-iframe-div" style="display:none" tabindex="-1" role="dialog">
Copy link
Contributor

Choose a reason for hiding this comment

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

avoid inline style if possible?

ern
ern reviewed Sep 5, 2024
View reviewed changes
basiclti/basiclti-tool/src/webapp/vm/lti_tool_edit.vm
#end

#if ($canDeploy)
<div style="float:right; display: none;" id="lti-deployment-div">
Copy link
Contributor

Choose a reason for hiding this comment

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

avoid inline style if possible?

ern
ern reviewed Sep 5, 2024
View reviewed changes
basiclti/basiclti-tool/src/webapp/vm/lti_tool_system.vm
#if (!$tool.get("SITE_ID") && $tool.get("visible") == 1)
## If SITE_ID is blank and the tool is stealthed, then it can be deployed
<li style="display:inline"><span aria-hidden="true"> / </span> <a href="$sakai_ActionURL.setPanel("ToolSiteDeploy")&tool_id=$tool.get("id")">$tlang.getString("gen.deploy")</a> </li>
<li style="display:inline"><a href="$sakai_ActionURL.setPanel("ToolEdit")&id=$tool.get("id")">$tlang.getString("gen.edit")</a></li>
Copy link
Contributor

Choose a reason for hiding this comment

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

avoid inline style if possible?

ern
ern reviewed Sep 5, 2024
View reviewed changes
basiclti/basiclti-tool/src/webapp/vm/lti_tool_system.vm
@@ -109,7 +109,11 @@ ${includeLatestJQuery}
#if ($messageSuccess)<div class="sak-banner-success">$tlang.getString("gen.success") $formattedText.escapeHtml($messageSuccess)</div>#end
#if ($alertMessage)<div class="sak-banner-error">$tlang.getString("gen.alert") $formattedText.escapeHtml($alertMessage)</div>#end
#if ($configMessage)<div class="sak-banner-error">$tlang.getString("gen.alert") $formattedText.escapeHtml($configMessage)</div>#end
#if ($isAdmin || $allowMaintainerAddSystemTool)<div align="right" style="padding:10px"><a href="" title="$tlang.getString("add.to.system")" onclick="location = '$sakai_ActionURL.setPanel("ToolInsert")';return false;"> $tlang.getString("add.to.system")</a>#if ($isAdmin )<br/><a href="" title="$tlang.getString("add.auto")" onclick="location = '$sakai_ActionURL.setPanel("AutoInsert")';return false;"> $tlang.getString("add.auto")</a>#end</div>#end
#if ($isAdmin || $allowMaintainerAddSystemTool)
<div align="right" style="padding:10px">
Copy link
Contributor

Choose a reason for hiding this comment

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

inline syle

ern
ern reviewed Sep 5, 2024
View reviewed changes
basiclti/basiclti-tool/src/webapp/vm/lti_tool_insert.vm
</div>

#if ($isAdmin)
<div id="siteId-input" class="foorm-text" style="clear:both;">
Copy link
Contributor

Choose a reason for hiding this comment

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

inline style

ern
ern reviewed Sep 5, 2024
View reviewed changes
basiclti/basiclti-tool/src/webapp/vm/lti_tool_insert.vm

#if ($isAdmin)
<div id="siteId-input" class="foorm-text" style="clear:both;">
<label for="siteId" style="display:block;float:none;font-weight:bold">$tlang.getString('tool.new.insert.start.siteId')</label>
Copy link
Contributor

Choose a reason for hiding this comment

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

inline style

ern
ern reviewed Sep 5, 2024
View reviewed changes
basiclti/basiclti-tool/src/webapp/vm/lti_tool_insert.vm
#if ($isAdmin)
<div id="siteId-input" class="foorm-text" style="clear:both;">
<label for="siteId" style="display:block;float:none;font-weight:bold">$tlang.getString('tool.new.insert.start.siteId')</label>
<div id="div_siteId"><input type="text" id="siteId" name="siteId" size="40" style="border:1px solid #555;padding:5px;font-size:1em;width:300px" value="$!previousPost.get("siteId")"/></div>
Copy link
Contributor

Choose a reason for hiding this comment

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

inline style

ern
ern reviewed Sep 5, 2024
View reviewed changes
basiclti/basiclti-tool/src/webapp/vm/lti_tool_insert.vm
<input type="hidden" name="sakai_csrf_token" value="$sakai_csrf_token" />
<input type="hidden" name="sakai_csrf_token" value="$sakai_csrf_token" />

<div id="title-input" class="foorm-text" style="clear:both;">
Copy link
Contributor

Choose a reason for hiding this comment

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

inline style

@csev csev merged commit 9c66d92 into sakaiproject:master Sep 5, 2024
4 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ern ern ern approved these changes

@csev csev csev approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@zhuoY121 @ern @csev