-
-
Notifications
You must be signed in to change notification settings - Fork 669
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
Link changes and in-table clipboards #4697
Conversation
httpstat.us appears to be down atm, thus the failing tests. Just as an FYI. |
@@ -106,7 +106,7 @@ <h4>{% trans "Build Details" %}</h4> | |||
<tr> | |||
<td><span class='fas fa-link'></span></td> | |||
<td>{% trans "External Link" %}</td> | |||
<td><a href="{{ build.link }}">{{ build.link }}</a>{% include "clip.html"%}</td> | |||
<td><a href="{{ build.link }}" rel="noreferrer" target="_blank">{{ build.link }}</a>{% include "clip.html"%}</td> |
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 think it would be worth breaking this out into a template e.g.
{% include "clip_link.html" with link=build.link %}
And then the template includes the clipboard functionality also.
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.
Yeah, I see what you mean.
Small braindump:
A general rule of thumb for clip_link.html will then be that it serves the URL as the string of the anchor tag.
In addition, it'll be targeted at links to be opened in new tabs, mainly external ones.
If a more generic approach would be ideal, more parameter for string value and external toggle could be added to the template.
LMK your thoughts, and I'll update the PR to use a template
These tests have been removed temporarily as the server seems to be really down. Merge in master and the tests should pass :) |
@LavissaWoW I'd like to restrict this PR just to the link / clipboard changes. There are some other points I'd like to discuss first on the concept of "generate link from SKU" |
@SchrodingersGat TBH, the idea itself has lessened somewhat in value in my mind after the above changes, and some of the modifications that were introduced with 0.11.0 My initial observation was that - in 0.10.0 - our purchaser was finding it tedious to find supplPart links from PO's. Thus, the original thought was born to be able to do a product search/lookup on the supplier's site with a single click. |
So this is not necessary again @LavissaWoW? Another note: have you looked into part/order metadata? Might also be a solution. |
This PR itself, as it stands, I still see a lot of value in terms of workflow and link security. But the base suggestion from the issue, as explained in my previous comment, has been largely mitigated with this PR and some of the changes that's present in 0.11. |
bf24e79
to
40fbcd8
Compare
@LavissaWoW sorry to be bothersome, but there's some weird CI issues currently. Can you please try merging inventree/master into your branch, and pushing again? |
* Clipboard icon added to tables for screens >1200px wide. Enables copying of SKU/MPN/IPN from table cells where these otherwise are hyperlinks * External links now open in new tabs with noreferrer
* All statically rendered external links have been moved out to a new template.
40fbcd8
to
64608c7
Compare
Thanks for the contribution :) |
Closes #4606
Clipboard icon added to tables for screens >1200px wide. Enables copying of SKU/MPN/IPN from table cells where these otherwise are hyperlinks
External links now open in new tabs with noreferrer