Skip to content
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

Add option to configure editor as non-editable #1206

Open
wants to merge 3 commits into
base: 9.x
Choose a base branch
from

Conversation

elpoelma
Copy link
Contributor

@elpoelma elpoelma commented Jul 18, 2024

Overview

This PR adds an option to configure the editor as non-editable.
It can be configured in two ways:

  • Using the ember Editor component, by passing the editable argument. The editor will also correctly react to value changes of this argument.
  • Using an instance of the SayController class, by executing the toggleEditable method

How to test/reproduce

  • Start the test-app
  • Open the index page
  • Test out the toggle editable button. This button uses the Editor component argument approach
  • The toggleEditable method of the editor controller should yield the same results.

Challenges/uncertainties

  • The toolbar/sidebar do not react to the editability of the document. At the moment, they should be configured to be enabled/disabled by the user of this addon.
  • Unsure if it is useful to keep both pathways to configure the editability of a document (controller vs component argument)

Checks PR readiness

  • UI: works on smaller screen sizes
  • UI: feedback for any loading/error states
  • Check cancel/go-back flows
  • Check database state correct when deleting/updating (especially regarding relationships)
  • changelog
  • npm lint
  • no new deprecations

@elpoelma elpoelma added the enhancement New feature or request label Jul 18, 2024
@elpoelma elpoelma self-assigned this Jul 18, 2024
@elpoelma elpoelma marked this pull request as draft July 19, 2024 08:26
@elpoelma elpoelma force-pushed the feat/add-non-editable-editor-option branch from feb0f59 to 1886d9f Compare July 19, 2024 10:06
@elpoelma elpoelma changed the base branch from master to 9.x July 19, 2024 10:06
@elpoelma elpoelma marked this pull request as ready for review July 19, 2024 10:06
Copy link
Contributor

@lagartoverde lagartoverde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works correctly, seems like it takes some time to make it editable again, but I guess the use of this is not to make editable/not editable quickly

@@ -111,6 +116,9 @@ export default class RdfaEditor extends Component<RdfaEditorArgs> {
nodeViews: this.args.nodeViews,
defaultAttrGenerators: this.args.defaultAttrGenerators,
keyMapOptions: this.args.keyMapOptions,
editable: () => {
return !(this.editable === false);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can do !!this.editable I think

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not in this case I think, as we do not want to trigger it when editable is undefined or null, this is why we strictly compare it to false.

@elpoelma elpoelma force-pushed the feat/add-non-editable-editor-option branch from 890c6a8 to 8cd349e Compare July 25, 2024 09:12
package.json Outdated Show resolved Hide resolved
@abeforgit
Copy link
Member

I'm missing also the visual changes we do in GN (placeholders from yellow to grey, etc)

I think we should try to make it as ootb as possible, requiring no extra css from embeddable users until they specifically ask for customization

@abeforgit
Copy link
Member

abeforgit commented Aug 5, 2024

With that said, we might even want to hide the toolbar and sidebar as well with this toggle

addon/core/say-controller.ts Outdated Show resolved Hide resolved
@elpoelma elpoelma force-pushed the feat/add-non-editable-editor-option branch from 8cd349e to 93d2eea Compare August 6, 2024 08:05
@elpoelma
Copy link
Contributor Author

elpoelma commented Aug 6, 2024

With that said, we might even want to hide the toolbar and sidebar as well with this toggle

I'm not sure if that is the responsibility of this addon? As the user can configure themselves whether to show the sidebar and toolbar, essentially this addon does not have control over their visibility. We can however, control that visibility in the embeddable package.

@elpoelma
Copy link
Contributor Author

elpoelma commented Aug 6, 2024

I'm missing also the visual changes we do in GN (placeholders from yellow to grey, etc)

I think we should try to make it as ootb as possible, requiring no extra css from embeddable users until they specifically ask for customization

Agreed, will look into adapting the css. We might then also have to do some adaptions of the plugins css.

@abeforgit
Copy link
Member

With that said, we might even want to hide the toolbar and sidebar as well with this toggle

I'm not sure if that is the responsibility of this addon? As the user can configure themselves whether to show the sidebar and toolbar, essentially this addon does not have control over their visibility. We can however, control that visibility in the embeddable package.

yeah ok fair enough

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants