-
Notifications
You must be signed in to change notification settings - Fork 416
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
#9180: Attribute Table in read only mode #9212
#9180: Attribute Table in read only mode #9212
Conversation
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.
-
change the flag
readOnlyAttribute
todisableFeaturesEditing
-
having a selector(Creator) that relies on a type of a plugin passed to get the proper layer rises the complxity of the solution without helping too much on future maintainence.
I suggest to refactor this solution by:
- keeping
isEditingAllowedSelector
untouched, without transforming it in a selector creator. This selector tells only if the editor is enabled in general. - remove
readOnlyAttributeSelector
that is hard to maintain (it relies on two plugins state) - Whenever a button that open the feature editor is present, add the additional check to hide it :
- you can add an additional utility function like
areFeaturesEditable
that checks the particular flag andsupportsFeatureEditing
that checks the type.
- you can add an additional utility function like
// utils
const supportedEditLayerTypes = [ "wms", "wfs"];
function supportsFeatureEditing(layer) {return includes(supportedEditLayerTypes, layer.type) };
function areLayerFeaturesEditable(layer) => !layer?.disableFeaturesEditing && supportsFeatureEditing(layer);}
For instance in IdentifyContainer
:
showEdit: showEdit && areLayerFeaturesEditable(layer) && isEditingAllowed && !!targetResponse && responseValidForEdit(targetResponse),
(and similar in feature grid toolbar).
This reduces a lot:
- the code to use
- the tests
- the maintainance
- the complexity
Please let me know what you think.
I agree. I initially thought of adding funcs as suggested but later thought it would be easier to manage in one place by a selector. Anyway, I will make the necessary modification. Thanks! |
trying to re-run |
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 selector is not needed anymore. Applying remove
When merged (it should auto-merge if coveralls responds), please @ElenaGalltest this on DEV. Thank you 🚀 |
@ElenaGallo, could you please test this on DEV ? Thank you |
Description
This PR adds a feature to layer setting to disable editing in Attribute table
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (check one with "x", remove the others)
Issue
What is the current behavior?
What is the new behavior?
Breaking change
Does this PR introduce a breaking change? (check one with "x", remove the other)
Other useful information
#7285 (comment)