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

Edge labels with border and spacing #1290

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mtutic
Copy link

@mtutic mtutic commented Jan 18, 2021

I found some time to work on #535. With font.borderColor, font.borderWidth and font.padding options, it is possible to add a border and space to the label. Unfortunatly, I didn't take care of the border radius.

I also don't know it this is the right way to do this but I tried it and it works.
Let me know if something is wrong and needs to be corrected!

@mtutic mtutic marked this pull request as ready for review February 5, 2021 13:36
Copy link
Member

@Thomaash Thomaash left a comment

Choose a reason for hiding this comment

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

Hi there,

first of all thanks for your work, however there are a few things that need to be done in order to have this merged:

  • There has to be some documentation for this.
  • It should be tested in some way (we have unit, function E2E and visual E2E already set up; pick whichever suits this the most and make sure it works as it should).
  • I think that padding should be a part of size (that's how CSS does it at the very least). The way it is now the edge can be covered by the border or the border can even extend to the other side of the edge. That's most likely not what most users would expect, especially if there was a background to the label as the label would be perceived as a single block and being misplaced (too close to the edge).
  • The border is sometimes rendered in the middle of nowhere and not around the label.
    Screen Shot 2021-02-06 at 19 00 06
    I couldn't figure out why but I got into this situation by modifying the basic usage example with the following code:
const edges = new vis.DataSet([
  { from: 1, to: 3, label: "EDGE" },
  { from: 1, to: 2, label: "EDGE" },
  { from: 2, to: 4, label: "EDGE" },
  { from: 2, to: 5, label: "EDGE" },
  { from: 3, to: 3, label: "EDGE" },
]);

const options = {
  edges: {
    font: {
      borderColor: "#ACDC00",
      borderWidth: 4,
      padding: 8,
      align: "top",
    },
  },
};
  • Another thing missing is validation. If I pass something like:
const options = {
  edges: {
    font: {
      borderColor: "Řecko",
      borderWidth: -44,
      padding: Number.NaN,
    },
  },
};

I should get a nice and explanatory message of what is wrong, not be left wondering why is nothing happening (in real use case these values could come from botched math expression, user inputs etc.).

  • Lastly, could you just fix the linting errors prior to opening a PR please? It should be as easy as running npm run style-fix && npm run lint-fix.

@kosirm
Copy link

kosirm commented Feb 11, 2024

Anything new? Is this going to be added? I need this urgently!

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.

None yet

3 participants