feat(ol-style-text): added backgroundFill and backgroundStroke props #223
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hey! First of all, thanks for this awesome library! :)
One thing I was missing is the ability to set
backgroundFill
andbackgroundStroke
options of ol/style/Text, so I added it.Description
backgroundFill
prop. It's type isColor | ColorLike
so it's easy to pass just a color when using the component.backgroundStoke
prop. It's type isStrokeOptions
. When using the component, we can pass a plain object with proper fields.createText
function's signature to be able to accept the new props. Originally it accepted a form ofOptions
as argument, but since the 2 new props doesn't match that, I had to change to the type of this component's props.createText
function to exclude the 2 new props as well from innerProperties, then add them as needed to Text's options.Usage:
I considered alternative approaches to add these 2 text style parameters, but found this one the easiest to implement and fit in the library.
Alternative B would be to add an
as-background
prop tool-style-fill
andol-style-stroke
, make them check if they are inside anol-style-text
, and depending on these 2 they can decide whether to callsetFill/setStroke
orsetBackgroundFill/setBackgroundStroke
. The problem with this is that if theas-background
prop is removed, they cannot set the visual style of Text reliably. They can only set it back to default, but if there's anotherol-style-fill
orol-style-stroke
besides the current one inside anol-style-text
slot, the visual style will be inconsistent.Alternative C would be to add new
ol-style-background-fill
andol-style-background-stroke
components which would only set the background fill and stroke, and only inside anol-style-text
. I found this also unnecessary complex and confusing asbackgroundFill
andbackgroundStroke
only exist inol/style/Text
in OpenLayers.However, I see that my current approach has a disadvantage that stroke options are squeezed into one prop.
Please say if you have a different view on how this feature should be added in the library and I can try to update the PR accordingly.
Motivation and Context
OlStyleText
already implemented all ofol/style/Text
's attributes except these 2, and it would be nice if we could set the background style of the texts too with this library.How Has This Been Tested?
I tested it by hand during development. I ran
npm run docs:dev
, navigated to http://localhost:5173/componentsguide/styles/text/ and editedTextDemo
to check how the text style reflects to code changes. Adding and changingbackground-fill
andbackground-stroke
props updates the visual style immediately, and removing them reverts the text background style to OL defaults.Don't think these changes should affect any other parts of the library, but please advise if I have to test anything and how.
Screenshots (if appropriate):
Types of Changes
Checklist:
If you added a new component feature (layer, geom, source, etc.), please be sure to update the documentation:
output.globals
invite.config.ts
src/demos/<Component>Demo.vue
docs/componentsguide/<Category>/<Feature>/index.md
containing the Demo and documentation for the componentdocs/.vitepress/config.ts
docs/public/sitemap.xml