-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Geolocation source configurable in map editor #2755
Geolocation source configurable in map editor #2755
Conversation
protected heading?: string; | ||
protected inputLabel?: string; | ||
|
||
static get properties(): PropertyDeclarations { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't use this format anymore, instead use decorators. See #2711
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK
|
||
return html` | ||
${this.renderStyle()} | ||
<h3>${this.heading}</h3> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This component shouldn't have its own heading, that reduces reusability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. I moved the heading to the map card editor
fireEvent(this, "value-changed"); | ||
} | ||
|
||
private renderStyle(): TemplateResult { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
static get styles(): CSSResult {
return css`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK
return html` | ||
<style> | ||
.entries { | ||
padding-left: 20px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't do this, this reduces reusability of this component.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I wanted to get the same style as the entity picker. I'll try to move this CSS to the map editor instead which is using the input list editor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, moved styling to map card editor
(ev.target! as LitElement).blur(); | ||
} | ||
|
||
private _valueChanged(ev: Event): void { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you should stop propagation on this event.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK
if (target.value === "") { | ||
newEntries.splice(target.index!, 1); | ||
} else { | ||
newEntries[target.index!] = target.value!; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should never change the input value, instead create a new value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I see. For that to work, the _valueChanged
method in the map card editor may require some more changes, because if target.configValue
is defined for the element, and target.value
is not changed, then the method immediately returns.
When I pass back the changed value through the event, it will be available as ev.detail.value
.
I'll look into this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just realised that even the standard paper-input
components return the new value in ev.detail.value
. Is there any particular reason why one would choose ev.target.value
over ev.detail.value
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to make a couple of changes to _valueChanged
of the map card editor to make this work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Of course now lint was complaining that EntitiesEditorEvent
's detail
doesn't have a value
. In an attempt to make the code cleaner, I split the event handler for the entity editor from the other input elements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, ok to merge when rebased.
dc82427
to
9f1fb27
Compare
9f1fb27
to
abc1154
Compare
Just rebased the code again, and made a small modification using the customElement decorator in the input list editor now. |
This will complete the map editor by making the geolocation source configurable in the UI. To achieve this, I implemented a simple list editor (
HuiInputListEditor
) which presents each source in a separate input field, with an additional empty input field at the end of the list for adding a new value.The
HuiInputListEditor
is fairly generic and could later be extended, for example to support editing a list of integers or a list of floats, but in this version it only support editing strings.