Repository navigation
Conversation
|
Hi @alixander, please review this PR once you have time. |
| "filled": {}, | ||
| } | ||
|
|
||
| var CompositeStyleKeywords = map[string]string{ |
There was a problem hiding this comment.
Currently, CompositeStyleKeywords is of type map[string]string, but in future if we support more styles under multiple we can convert it to map[string][]string to hold more values under one style.
There was a problem hiding this comment.
we shouldn't need this. for example, the ability to do label.near is implemented, but there's no special map for that.
| } | ||
|
|
||
| func compileCompositeStyleFieldInit(styles *d2graph.Style, f *d2ir.Field, parent string) { | ||
| switch f.Name.ScalarString() { |
There was a problem hiding this comment.
I coundn't find an way to fetch parent value for current keyword, so defined a new function instead of using the existing func compileStyleFieldInit, since we needed an extra parameter holding the value of parent. We can use the existing function also, but have to include a 3rd parameter parent.
There was a problem hiding this comment.
Similar to the other comment, since nested keywords are already implemented elsewhere, you should question why you'd need a new function for this case.
| return nil | ||
| } | ||
|
|
||
| func (s *Style) ApplyComposite(key, value, parent string) error { |
There was a problem hiding this comment.
Similar to compileStyleFieldInit, we need an extra paramater holding the parent value here also, so created a new func ApplyComposite.
|
Hi @alixander, please review! |
| "filled": {}, | ||
| } | ||
|
|
||
| var CompositeStyleKeywords = map[string]string{ |
There was a problem hiding this comment.
we shouldn't need this. for example, the ability to do label.near is implemented, but there's no special map for that.
| } | ||
|
|
||
| func compileCompositeStyleFieldInit(styles *d2graph.Style, f *d2ir.Field, parent string) { | ||
| switch f.Name.ScalarString() { |
There was a problem hiding this comment.
Similar to the other comment, since nested keywords are already implemented elsewhere, you should question why you'd need a new function for this case.
Hey @alixander, no worries at all. I noticed you were inactive for a couple of weeks, I just put the comment to notify you once you cone back. And welcome back btw : ) |
|
Regarding the comments, I'll look into it and come up with an efficient solution. |
There was a problem hiding this comment.
the test results aren't right. this shouldn't have changed
| if f.Map() != nil { | ||
| for _, ff := range f.Map().Fields { | ||
| if ff.Name.ScalarString() == "opacity" && ff.Name.IsUnquoted() { | ||
| if ff.Primary() == nil { | ||
| c.errorf(ff.LastPrimaryKey(), `invalid "opacity" field`) | ||
| } else { | ||
| scalar := ff.Primary().Value | ||
| switch f.Name.ScalarString() { | ||
| case "multiple": | ||
| styles.MultipleOpacity = &d2graph.Scalar{MapKey: f.LastPrimaryKey()} | ||
| err := styles.Apply(f.Name.ScalarString()+ff.Name.ScalarString(), scalar.ScalarString()) | ||
| if err != nil { | ||
| c.errorf(scalar, err.Error()) | ||
| return | ||
| } | ||
| default: | ||
| c.errorf(f.Name, `invalid "opacity" style for "%s"`, f.Name.ScalarString()) | ||
| return | ||
| } | ||
| } | ||
| } else { | ||
| if ff.LastPrimaryKey() != nil { | ||
| c.errorf(ff.LastPrimaryKey(), `unexpected field %s`, ff.Name.ScalarString()) | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
it should be a code smell to you that you need this much extra code. There's a more precise place to make these changes
There was a problem hiding this comment.
Sure, we can simplify the process by modifying
https://github.com/terrastruct/d2/blob/b2841d586f6c2b238b677b4043f7c8cadfc5f266/d2compiler/compile.go#L788-L792
as:
func (c *compiler) compileStyle(styles *d2graph.Style, m *d2ir.Map) {
for _, f := range m.Fields {
c.compileStyleField(styles, f)
if f.Map() != nil {
for _, ff := range f.Map().Fields {
c.compileStyleField(styles, ff)
}
}
}
}However, as it’s not possible to get the parent field of a particular field, we need to pass a new parameter to
https://github.com/terrastruct/d2/blob/b2841d586f6c2b238b677b4043f7c8cadfc5f266/d2compiler/compile.go#L794
to identify the parent field for opacity.
| return fmt.Errorf(`expected "text-transform" to be one of (%s)`, strings.Join(d2ast.TextTransforms, ", ")) | ||
| } | ||
| s.TextTransform.Value = value | ||
| case "multipleopacity": |
There was a problem hiding this comment.
where does this come from?
There was a problem hiding this comment.
Since we only have access to the key and value in the Apply function, if we pass only the "opacity" keyword as the key, there’s no way to distinguish whether it specifies the global-level opacity or multiple opacity.
This branch has not been deployed
3 participants
Added ability to face the background shape when multiple style is specified.
/fixes #1516