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

[editor] Introduce a proper annotationEditorMode option/preference (PR 15075 follow-up) #15113

Merged
merged 3 commits into from
Jun 29, 2022

Conversation

Snuffleupagus
Copy link
Collaborator

  • [editor] Introduce a proper annotationEditorMode option/preference (PR 15075 follow-up)

    This replaces the boolean annotationEditorEnabled option/preference with a "proper" annotationEditorMode one. This way it's not only possible for the user to control if Editing is enabled/disabled, but also which specific Editing-mode should become enabled upon PDF document load.

    Given that Editing is not enabled/released yet, I cannot imagine that changing the name and type of the option/preference should be an issue.

  • [editor] Remove the unused name-property from the editorParamsToolbars DOM elements

    As far as I can tell, this is completely unused and can thus be removed.

  • [editor] Slightly shorten the en-US freetext_default_content placeholder text

    Now that it's possible to change the font-size, this placeholder string feels a little bit long (especially for larger font-sizes).

    Given that Editing is not enabled/released yet, I hope that it should be fine to update this without changing the l10n-id.

…(PR 15075 follow-up)

This replaces the boolean `annotationEditorEnabled` option/preference with a "proper" `annotationEditorMode` one. This way it's not only possible for the user to control if Editing is enabled/disabled, but also which *specific* Editing-mode should become enabled upon PDF document load.

Given that Editing is not enabled/released yet, I cannot imagine that changing the name and type of the option/preference should be an issue.
…ars DOM elements

As far as I can tell, this is completely unused and can thus be removed.
…older text

Now that it's possible to change the font-size, this placeholder string feels a little bit long (especially for larger font-sizes).

Given that Editing is not enabled/released yet, I hope that it should be fine to update this without changing the l10n-id.
@Snuffleupagus Snuffleupagus changed the title [editor] Introduce a proper annotationEditorMode option/preference (PR 15075 follow-up) [editor] Introduce a proper annotationEditorMode option/preference (PR 15075 follow-up) Jun 29, 2022
Copy link
Contributor

@calixteman calixteman left a comment

Choose a reason for hiding this comment

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

LGTM, thank you.

@Snuffleupagus
Copy link
Collaborator Author

/botio integrationtest

@pdfjsbot
Copy link

From: Bot.io (Windows)


Received

Command cmd_integrationtest from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.193.163.58:8877/1365024cc0d6936/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Received

Command cmd_integrationtest from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.241.84.105:8877/8c26b2eded5e1fa/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Success

Full output at http://54.241.84.105:8877/8c26b2eded5e1fa/output.txt

Total script time: 4.72 mins

  • Integration Tests: Passed

@pdfjsbot
Copy link

From: Bot.io (Windows)


Failed

Full output at http://54.193.163.58:8877/1365024cc0d6936/output.txt

Total script time: 7.59 mins

  • Integration Tests: FAILED

@Snuffleupagus Snuffleupagus merged commit e08b079 into mozilla:master Jun 29, 2022
@Snuffleupagus Snuffleupagus deleted the annotationEditorMode-pref branch June 29, 2022 11:40
@calixteman
Copy link
Contributor

@Snuffleupagus, there is a leftover:

compatibilityParams.annotationEditorEnabled = false;

@Snuffleupagus
Copy link
Collaborator Author

@Snuffleupagus, there is a leftover:

compatibilityParams.annotationEditorEnabled = false;

Oops; I'll fix that, and also ensure that the default value is handled correctly in the BaseViewer-constructor. (Thankfully all of this only matters for old and outdated Safari.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants