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

Bookmarks features and changes for issue #40 #42

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

sajarix
Copy link

@sajarix sajarix commented Oct 10, 2024

Resolving issue #2, #39 and #40

@gh4nh
Copy link
Contributor

gh4nh commented Nov 22, 2024

Test comment 22. Nov.

Copy link
Collaborator

@CraigRubendall CraigRubendall left a comment

Choose a reason for hiding this comment

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

several items that should be changed/fixed before accepting this request.

@@ -484,14 +486,33 @@
</xsl:variable>
<xsl:variable name="stpURI"><xsl:text>/SASStoredProcess/do?_action=form,properties,execute&amp;_program=</xsl:text><xsl:value-of select="$stpProgram"/></xsl:variable>

<a><xsl:attribute name="href"><xsl:value-of select="$stpURI"/></xsl:attribute><xsl:value-of select="@Name"/></a>
<xsl:variable name="stpLocation">
Copy link
Collaborator

Choose a reason for hiding this comment

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

calculating this path is an expensive operation, so we should try to do it as few times as possible.

I would recommend moving this variable stpLocation setting up, before the setting of stpProgram, and then change the setting of stpProgram to be

<xsl:value-of select="$stpLocation"/>xsl:text/</xsl:text><xsl:value-of select="@name"/>

so as to not have to calculate the path twice

Copy link
Author

Choose a reason for hiding this comment

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

I'm going to do the adjustment as you have suggested.

<a><xsl:attribute name="href"><xsl:value-of select="$wrsURI"/></xsl:attribute><xsl:value-of select="@Name"/></a>
<a><xsl:attribute name="href"><xsl:value-of select="$wrsURI"/></xsl:attribute><xsl:value-of select="@Name"/></a><br/>

<xsl:variable name="stpLocation">
Copy link
Collaborator

Choose a reason for hiding this comment

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

chose a variable name here consistent with the others in this template, ex. wrsLocation, instead of the one copied from stp.

Copy link
Author

Choose a reason for hiding this comment

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

I'm going to do the adjustment as you have suggested.

@@ -874,6 +874,7 @@
{
selBox = document.getElementById(name);
selectAllOptions(selBox);
window.location.href = sasPortalAppHome;
Copy link
Collaborator

Choose a reason for hiding this comment

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

What exactly is this code doing? Why are we setting the window href here? That will force a redirect, correct?

Copy link
Author

@sajarix sajarix Nov 25, 2024

Choose a reason for hiding this comment

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

If we add a new item to the element and press OK, the new item won't appear in the portal. So this line forces a reload of the page. To do this we need the Java script variable "var sasPortalAppHome="/SASPortalApp";". Alternatively, we can store the URL in the session stoarage and then use it from there.

/*
* Allow overrides of default Portal URL pathname
*/
var sasPortalAppHome="/SASPortalApp";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this needed?

Copy link
Collaborator

Choose a reason for hiding this comment

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

This file looks like an exact copy of the update.collections.xslt file. If that is so, then we should rework the collections and bookmark update to both call a shared file.

When creating the shared file, you have to be careful with the paths to load localizations, etc. I don't see that the file has any localizations, but if it needs it, you can see an example of how this works, see the remove.xxx.xslt files and how they reference the shared remove.psportlet.xslt file (and how that shared file accesses the localizations).

Copy link
Author

Choose a reason for hiding this comment

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

I'm going to do the adjustment as you have suggested.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants