-
-
Notifications
You must be signed in to change notification settings - Fork 228
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
feat: very minimal experimental conditional style rules #1776
Conversation
yohanboniface
commented
Apr 25, 2024
aa89dba
to
6a0d842
Compare
@Aurelie-Jallut some screens: |
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.
Here are just a few comments while reading the code :-) Cool, we'll have filtering rules soon :-)
|
||
class Rule { | ||
constructor(map, condition = '', options = {}) { | ||
this.map = map |
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.
If these are private, should we prefix them with #
?
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.
Sadly, the browser support is not enough for us for this feature yet :(
|
||
class Rule { | ||
constructor(map, condition = '', options = {}) { | ||
this.map = map |
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'm wondering if it would it be possible to not pass the map
object here? You would need to find ways to handle isDirty
and the render
call in a different way, maybe with events?
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 is a pattern we already have elsewhere in uMap, including in modules. Also I remember removing events call from the requests
module (done by inheriting Leaflet Class
), so I'm not exactly sure what is the pattern we'd encourage.
Anyway, I'm ok to challenge this pattern, but I'd prefer to do it in a separate PR and not to charge this one.
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.
The goal would be to move away from having this global map
superobject. I'm okay to follow the discussion elsewhere.
import * as Utils from './utils.js' | ||
import { translate } from './i18n.js' | ||
|
||
class Rule { |
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.
It could be useful to have a comment explaining the API for these two classes.
0955cee
to
a424ab8
Compare
25ec622
to
86c15f2
Compare
3b6861a
to
bad5daf
Compare
And make sure we redraw data on map
Support is not ready for us.
Browser support is not enough.
bad5daf
to
c7541c1
Compare