Conversation
bd44e84 to
4640a4e
Compare
Bumps [@commitlint/cli](https://github.com/conventional-changelog/commitlint/tree/HEAD/@commitlint/cli) from 20.2.0 to 20.3.0. - [Release notes](https://github.com/conventional-changelog/commitlint/releases) - [Changelog](https://github.com/conventional-changelog/commitlint/blob/master/@commitlint/cli/CHANGELOG.md) - [Commits](https://github.com/conventional-changelog/commitlint/commits/v20.3.0/@commitlint/cli) --- updated-dependencies: - dependency-name: "@commitlint/cli" dependency-version: 20.3.0 dependency-type: direct:development update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com>
4640a4e to
22cd58a
Compare
a9658ca to
9a5d3cf
Compare
9a5d3cf to
2fa72f1
Compare
# Conflicts: # package-lock.json # src/core/types/plugin.ts
|
There was a problem hiding this comment.
- I can't seem to be able to tab through the selection of the buttons. Please take a look at this. This probably results from the
<input>s being hidden and only the labels being displayed. Not sure how a ScreenReader would interpret them as well. Maybe using an actual button here seems reasonable? - Selecting a BlockButton makes it jiggle a bit, which is annoying. This should be fixable by reserving some space for the
border. - Implement the old UI for
layout: 'nineRegions'. As we didn't have a final draft yet, the previous layout should be available.
I've also attached a patch which adds the pointer-events to the correct elements.
src/plugins/filter/types.ts
Outdated
| /** | ||
| * Technical value of a feature property. | ||
| */ | ||
| value: string |
There was a problem hiding this comment.
It seems fitting to also allow multiple values here.
There was a problem hiding this comment.
This type describes a single entry (checkbox) for a category. As far as I understand the POLAR@2 documentation on this (and the requirements spec), this should be a single value.
Why would you allow multiple values here? Is the intention to group multiple values to a single option?
There was a problem hiding this comment.
knownValues previously was of type string[], so we didn't have this situation yet.
It seemed fitting to be able to group multiple known values as a single option to reduce e.g. an almost endless list. This is not essential though 🎩
I cannot access that file...feel free to commit it directly on the branch. |
|
|
Old UI = visible checkboxes and radio buttons? |
| } | ||
|
|
||
| function getLayerConfiguration(layerId: string) { | ||
| const polar = configuration.value.layers.find( |
There was a problem hiding this comment.
Wouldn't mapConfiguration be more precise?
src/core/stores/index.ts
Outdated
|
|
||
| /** | ||
| * Returns the layer configuration or `null` if the layer was not found. | ||
| * The configuration is merged from the service register and the POLAR `layers` configuration. |
There was a problem hiding this comment.
Linking {@link createMap | serviceRegister parameter of createMap} and MapConfiguration.layers seems reasonable
| .polar-filter-card { | ||
| pointer-events: all; | ||
| padding-bottom: 3em; | ||
| } |
There was a problem hiding this comment.
Not needed. Just makes the whole card focusable as well.
Don't forget to remove the class on the card as well.
src/core/stores/main.ts
Outdated
| if (!polar || !register) { | ||
| return null | ||
| } | ||
| return { ...register, ...polar } |
There was a problem hiding this comment.
The return should have some other type than any
src/plugins/filter/stores/main.ts
Outdated
| watch(selectedLayerId, (newLayerId) => { | ||
| if (newLayerId === null || state.value[newLayerId]) { | ||
| return | ||
| } | ||
| state.value[newLayerId] = {} | ||
| }) |
There was a problem hiding this comment.
Could this be done on setup instead? Or is there a reason behind doing this "lazily"? It would also ease the type for selectedLayerState
For some reasons not further researched Gecko sometimes allows focusing a PolarCard. This occured before this commit in the filter plugin. To fix this, the element is explicitly removed from regular tab order.

Summary
The filter plugin is migrated to POLAR@3.
Instructions for local reproduction and review
Additional hints
Layout flawsThe layout of the filter dialog is somehow broken.It is larger than the map's height (in the snowbox).
You may scroll it, but you cannot see the bottom of the scrollbar, as it overflows.
This, however, is, as far as I can tell, not a filter-specific problem but located probably in iconMenu.Therefore, it is not considered here.
Fixed with #493
Styling
The general styling for this component is inspired by some (deprecated) UI/UX drafts.
Date range picker
The date range picker consists of two browser-native date pickers.
This control is supported by all major browsers (https://caniuse.com/input-datetime).
This improvised date range picker already supports limiting the dates (up to or starting with today's date).
An advanced implementation (as before with Vuetify) is left open to another PR.
Relevant tickets, issues, et cetera
Closes #370