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

10011 add node alias and widget name as class to HTML elements #10034

Merged
merged 5 commits into from
Oct 11, 2023

Conversation

bferguso
Copy link
Contributor

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Description of Change

Adds the associated node alias and widget name as classes to HTML container elements to allow specific node elements to be targeted by CSS rules.

Implementation Notes

  • Currently just added at a top level div for each widget/report/display_value section to not litter many HTML elements with the same classes.
  • report elements do not appear to have a top level div so applied to the dt, and dd elements.
  • widget name is only available when in the form elements are visible.
  • node_alias is available for form, report and display_value blocks.
  • left config_form section out of scope as this is primarily intended for user presentation.

Issues Solved

#10011

Checklist

  • Unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

Ticket Background

Further comments

@bferguso bferguso changed the base branch from master to dev/7.5.x September 12, 2023 20:47
@chiatt chiatt self-assigned this Sep 14, 2023
Copy link
Member

@chiatt chiatt left a comment

Choose a reason for hiding this comment

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

This looks really good. I'm wondering if it would make sense to include the graph slug in the nodeCssClasses. That way if you had a node alias shared by two or more resource models, you could style them differently.

@bferguso
Copy link
Contributor Author

This looks really good. I'm wondering if it would make sense to include the graph slug in the nodeCssClasses. That way if you had a node alias shared by two or more resource models, you could style them differently.

@chiatt - good idea. As far as I can see the graph slug doesn't seem to be available in the widget.js component right now. There is a graph object available through the form (only when the edit widgets are available) but it doesn't include the slug. We'll need to figure out how to make it available so we can include it as a class. If you know off the top how we can do that I'd be interested in any suggestions. Otherwise I'll dig into that when I have a minute.

@chiatt chiatt merged commit 2f39375 into dev/7.5.x Oct 11, 2023
@chiatt chiatt deleted the 10011_target_widget_css branch October 11, 2023 21:33
@chiatt
Copy link
Member

chiatt commented Oct 11, 2023

I've added an issue for the graph slug implementation so that we can handle it in a different PR: #10119

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