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

Add href attribute to connectors, cables, and additional_bom_items #168

Draft
wants to merge 3 commits into
base: dev
Choose a base branch
from

Conversation

kvid
Copy link
Collaborator

@kvid kvid commented Sep 14, 2020

Implement second method in #165

@formatc1702 formatc1702 added this to the v0.3 milestone Oct 17, 2020
@formatc1702
Copy link
Collaborator

I like this feature!
How about using url instead of href for this new attribute? It sounds more natural and doesn't have the "early 90s HTML" vibe to it ;)

@formatc1702
Copy link
Collaborator

formatc1702 commented Oct 23, 2020

IMHO, this is a good candidate for merging soon, after #163, or before, if that one requires additional discussion. There shouldn't be any conflicts between the two.
Feel free to start rebasing onto dev and, if you don't disagree, implement my previous comment.
Thanks!

@kvid
Copy link
Collaborator Author

kvid commented Oct 23, 2020

IMHO, this is a good candidate for merging soon, after #163, or before, if that one requires additional discussion. There shouldn't be any conflicts between the two.
Feel free to start rebasing onto dev and, if you don't disagree, implement my previous comment.
Thanks!

  • I've started rebasing onto dev, but it was more work that expected to adapt the new attribute all the new places after the recent changes in dev. Feature/additional components #115 helped reducing the number of places in the code each attribute is referenced, but there are still too many places in the code involved for a single change of or adding an attribute. It would have so much easier to add href only once in BaseComponent with a data structure suggested in [feature] More control over wire parameters #56 (comment).
  • I also detected a possible bug that Graphviz copies href from the previous edge unless href is specified as '' instead of None. I need to investigate both nodes and edges for this issue.
  • I'm considering a default value for href: Copying the href from a link in a text attribute (the first one found if there are several). That would let the user define an URL only once to obtain a link in all outputs - possibly without the need for a long literal URL column in the HTML BOM.
  • Rename href to url is probably a good idea, but there are some other issues first in line.
  • I started with this one because I thought it was the easy one, but I should probably put this on hold and finish Update the types of dataclass attributes according to usage #163 first.

@formatc1702
Copy link
Collaborator

formatc1702 commented Oct 26, 2020

Take your time with this, it's not critical.

  • I'm considering a default value for href: Copying the href from a link in a text attribute (the first one found if there are several). That would let the user define an URL only once to obtain a link in all outputs - possibly without the need for a long literal URL column in the HTML BOM.

Would it make sense to turn this around? #164 was made before the idea of a separate url attribute was suggested in #165.
It would be more natural to define the URL in its own field, and then, if needed, that value could be referenced inside other fields.

Example from #164 (alternatives to $URL as a placeholder are welcome)

# Example 7: Crossover Cable
connectors:
  X1:
    type: '<a href="$URL">Stewart Connector SS-37000-002</a>'
    subtype: male
    pinlabels: [DA+,DA-,DB+,DC+,DC-,DB-,DD+,DD-] # pincount is implicit in pinout
    url: https://www.digikey.com/product-detail/en/stewart-connector/SS-37000-002/380-1098-ND/1033365
  X2:
    type: '<a href="$URL">Stewart Connector SS-37000-002</a>'
    subtype: male
    pinlabels: [DB+,DB-,DA+,DD+,DD-,DA-,DC+,DC-]
    url: https://www.digikey.com/product-detail/en/stewart-connector/SS-37000-002/380-1098-ND/1033365

TBH, having a separate url field will probably make the need for including URLs in the type attribute obsolete in most cases.

The HTML BOM could have a column that just shows the text [Link] or similar, as a hyperlink to the actual URL.

If someone really needs the full URL visibly listed as text inside the BOM, a new global harness parameter could take care of that; something like show_full_url_in_bom: true but hopefully with a shorter name ;) Could be useful when printing out the BOM; but even then I doubt someone will want to type the URL back in manually, so it should not be the default behavior.

@formatc1702
Copy link
Collaborator

(the first one found if there are several)

If desired, referencing multiple URLs could be achieved by allowing url to be both a str or a List of strs, and then referencing each entry using $URL[1] or similar. The BOM could list all the URLs in the same column as [Link 1] [Link 2] ....

I see this as an edge case, however.

@formatc1702
Copy link
Collaborator

formatc1702 commented Oct 26, 2020

FYI, GraphViz HTML supports links inside tables, not as separate <A> tags, but as part of the <TD> tag:

<td href="http://example.com">&#x1F517;</td>

I would be OK for including clickable links within the GraphViz output, under the condition that they are discrete, and don't look ridiculous when, for example, printing the diagram, rendering them useless. So, having some discrete 🔗 icon or similar in the corner is OK; having a separate cell with the word 'Link` in it is not.

Just leaving it here as an additional idea.

[Edit] Sorry if you feel this should go in a separate issue/PR... let me know.

@formatc1702
Copy link
Collaborator

I noticed you mentioned this PR in #189 (comment), but it is still marked as draft. So I'm not sure if you want me to take a closer look already, or if you're still working on it :) Either way, simply change the draft status and drop a comment when it's ready! No pressure, just wanting to avoid any misunderstanding.

@kvid
Copy link
Collaborator Author

kvid commented Dec 26, 2020

I noticed you mentioned this PR in #189 (comment), but it is still marked as draft. So I'm not sure if you want me to take a closer look already, or if you're still working on it :) Either way, simply change the draft status and drop a comment when it's ready! No pressure, just wanting to avoid any misunderstanding.

I've edited my comment you are referring to and made it more clear that I don't expect any review + merge-in before I complete this draft and mark it ready for review.

It's still easy to rebase this branch onto the current dev branch, but I'm struggling a bit to generalize handling both line breaks and HTML tags in attributes from the YAML input in a uniform manner. When containing HTML tags, line breaks should be used to split the HTML into several lines in a readable manner, but great care must be taken to avoid conflicts with the feature to convert all line breaks into <BR/> tags. I think we should reconsider this latter feature. Maybe the need to visually split text lines in the diagram can be specified differently to avoid such conflicts and simplify the implementation?

@formatc1702
Copy link
Collaborator

Maybe the need to visually split text lines in the diagram can be specified differently to avoid such conflicts

Can you elaborate on this? Do you mean something like an additional attribute to enable/disable "smart" splitting of URLs in to lines?

If breaking apart long URLs is the only thing blocking this PR, I would suggest just printing URLs in one line for now, and tackling the issue in a separate PR.

In addition, please consider the following ideas. Bear in mind that they are not 100% thought out, it's just a spontaneous thought.

Idea 1
It might not be an ideal solution, but would it make sense to completely hide any URLs defined in this href attribute behind a link icon or similar, as mentioned above? Maybe have an additional show_href or similar, defaulting to False, but with the possibility to set to True in case the user really requires the URL to be printed out?

Idea 1.1
Perhaps a bit unconventional, but instead of the href and show_href suggested above, the user could provide a visible_href (if they need the URL printed explicitly) or hidden_href (if a clickable link icon is enough) attribute in the YAML, that internally gets mapped to href and the corresponding show_href boolean. This would make the YAML more readable.

@kvid
Copy link
Collaborator Author

kvid commented Feb 9, 2021

Maybe the need to visually split text lines in the diagram can be specified differently to avoid such conflicts

Can you elaborate on this? Do you mean something like an additional attribute to enable/disable "smart" splitting of URLs in to lines?

What I have in mind, is whether it would be better to let the user choose between text that might contain linebreaks that he wants to be translated to <br/>, or text that might contain links or other tags to be used in HTML output. In the latter case, any <br/> must be inserted by the user himself, and linebreaks are not translated, but used as they are to obtain readable HTML.

Either this choice can be a global option as in #158, or maybe per attribute like this:

    mpn:
        html: '<a href="...">00893-11</a><br/>(temporary out of stock)'
    pn:
        plain: "43-456-700\n(limited local stock)"

The concept above also open up for more than one alternative per attribute, e.g. both HTML alternative and non-HTML alternative, and even one alternative for each of the outputs (gv, html, tsv, etc.) if that could be useful for anyone.

Another way, could be to create a rule for an automatic choice, saying text containing any tag <[a-zA-Z].*> is regarded as HTML and no linebreak translation is done, and only translate linebreaks to <br/> in texts containing no tags.

If breaking apart long URLs is the only thing blocking this PR, I would suggest just printing URLs in one line for now, and tackling the issue in a separate PR.

Long URLs in the BOM is a separate issue that can be postponed into a separate issue as you suggest.

In addition, please consider the following ideas. Bear in mind that they are not 100% thought out, it's just a spontaneous thought.

Idea 1
It might not be an ideal solution, but would it make sense to completely hide any URLs defined in this href attribute behind a link icon or similar, as mentioned above? Maybe have an additional show_href or similar, defaulting to False, but with the possibility to set to True in case the user really requires the URL to be printed out?

Alternatively a global option.

Idea 1.1
Perhaps a bit unconventional, but instead of the href and show_href suggested above, the user could provide a visible_href (if they need the URL printed explicitly) or hidden_href (if a clickable link icon is enough) attribute in the YAML, that internally gets mapped to href and the corresponding show_href boolean. This would make the YAML more readable.

Also possible. I have to think about this one.

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.

2 participants