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

show (route) relation colours in lists and comboboxes #9424

Merged
merged 3 commits into from
Nov 17, 2023
Merged

Conversation

tyrasd
Copy link
Member

@tyrasd tyrasd commented Dec 12, 2022

Looks like this in the relation-membership section:

The route colour is similarly also shown in the list of search results.

@tyrasd tyrasd added the usability An issue with ease-of-use or design label Dec 12, 2022
@tyrasd tyrasd force-pushed the relation-colours branch 2 times, most recently from 8b076dd to c2157ba Compare December 12, 2022 21:21
@@ -349,6 +371,8 @@ export function uiSectionRawMembershipEditor(context) {
labelLink
.append('span')
.attr('class', 'member-entity-name')
.classed('has-colour', d => d.relation.tags.colour)
.style('border-color', d => d.relation.tags.colour)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we do any validation on the colour value? As far as I know, the value should be a hex triplet or CSS color keyword, but several other valid CSS color value syntaxes aren’t valid as tag value.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, some lines are identified by a combination of colors, which would likely be tagged as a semicolon-delimited list. We could show just the first, which could be ambiguous in some cases, or we could attempt to display a color swatch for each, or a gradient perhaps.

Copy link
Contributor

@Zaczero Zaczero Jan 24, 2023

Choose a reason for hiding this comment

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

@1ec5
As to your first question, I believe it is unnecessary. In the vast majority of cases, the values are CSS-compliant, and for the rest, I don't think putting in extra work just to handle those would be beneficial. This could be added down the line.

I misunderstood your question during the first read. I can see how users would start using non-standard color values just to make it 'look nice' in the iD. I do support the idea of putting some additional validation for the colors before rendering them out.

Maybe such regex would suffice: ^(#[0-9a-fA-F]{3}|#[0-9a-fA-F]{6}|[a-zA-Z]{3,})$.
It matches either: short hex triplet, hex triplet, or a word with at least 3 characters.
Or, just check how this has been done in the already-existing color field.


To your second question, it is a cool idea that is definitely worth noting as an enhancement issue, but I don't think this lack of support should block this PR. It is non-critical, just a nice-to-have. Anyway, I wrote a small code PoC:

function stripedGradient(colors) {
  var colorArray = colors.split(";");
  var colorStops = "";
  var stopIncrement = 100 / (colorArray.length);
  var currentStop = 0;
  
  for (var i = 0; i < colorArray.length; i++) {
    colorStops += "," + colorArray[i] + " " + currentStop + "%," + colorArray[i] + " " + (currentStop+stopIncrement) + "%";
    currentStop = Math.min(currentStop + stopIncrement + 0.001, 100);
  }
  
  return "linear-gradient(180deg" + colorStops + ")";
}

var cssColors = 'red;blue;black';
var gradient = stripedGradient(cssColors);

image

Copy link
Collaborator

Choose a reason for hiding this comment

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

Cool, I agree that handling multiple values could be tail work, though your PoC seems well on its way toward a solution.

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder whether this could be applied to 'normal' color field as well:

image

If this PR gets merged I can make a new one with adding striped colors support to both of them.

Copy link
Collaborator

@1ec5 1ec5 Feb 9, 2023

Choose a reason for hiding this comment

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

I misunderstood your question during the first read. I can see how users would start using non-standard color values just to make it 'look nice' in the iD. I do support the idea of putting some additional validation for the colors before rendering them out.

Correct, this was my main feedback. While it would be cool to be able to specify the RGBA or HSLA value of a glass-surfaced bus shelter, I think that would need to start with at least an informal tagging discussion rather than mappers stumbling upon iD’s accidental support for it and latching onto it.

field

member

@tyrasd tyrasd added this to the 2.27 milestone May 26, 2023
@tyrasd tyrasd merged commit d4bfbaa into develop Nov 17, 2023
4 checks passed
@tyrasd tyrasd deleted the relation-colours branch November 17, 2023 13:08
bhousel added a commit to facebook/Rapid that referenced this pull request Nov 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
usability An issue with ease-of-use or design
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants