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 customizable side-by-side mode to HTMLIntegration #5604

Merged
merged 1 commit into from
Jul 7, 2023

Conversation

acelaya
Copy link
Contributor

@acelaya acelaya commented Jul 6, 2023

This PR introduces options in HTMLIntegration, designed for host applications to have more control over side-by-side mode.

Currently, the only supported option is mode, which can take these values:

  • auto: It's the client's default value. It will apply some heuristics to determine how the content is affected.
  • manual: expresses the intention of the host app to take control of side-by-side, disabling the logic in the client.

This PR does not cover

  • How host apps provide these options: Side by side host config #5605
  • How these options apply to other integrations (PDF and VitalSource). We won't support this for now

These will come in follow-up PRs and discussions.

This PR is part of #5571

@codecov
Copy link

codecov bot commented Jul 6, 2023

Codecov Report

Merging #5604 (021834e) into main (fc5b760) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #5604      +/-   ##
==========================================
+ Coverage   99.42%   99.43%   +0.01%     
==========================================
  Files         239      239              
  Lines        9421     9427       +6     
  Branches     2229     2236       +7     
==========================================
+ Hits         9367     9374       +7     
+ Misses         54       53       -1     
Impacted Files Coverage Δ
src/annotator/integrations/html.ts 100.00% <100.00%> (ø)

... and 16 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@acelaya acelaya force-pushed the side-by-side-mode branch 3 times, most recently from 2aabbbe to 456ecce Compare July 6, 2023 08:31
@acelaya acelaya requested a review from robertknight July 6, 2023 08:34
@acelaya acelaya marked this pull request as ready for review July 6, 2023 08:34
@acelaya acelaya force-pushed the side-by-side-mode branch 2 times, most recently from 3accd6f to 8e28fb4 Compare July 6, 2023 09:33
Copy link
Member

@robertknight robertknight left a comment

Choose a reason for hiding this comment

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

The configuration interface looks good. I think the logic around merging the feature flag and configuration states can potentially be simplified. See my notes.

src/annotator/integrations/html.ts Outdated Show resolved Hide resolved
src/annotator/integrations/html.ts Outdated Show resolved Hide resolved
@robertknight
Copy link
Member

How these options apply to other integrations (PDF and VitalSource).

I think the answer here can be that we just don't support this flag if other integrations are used. In both cases we are effectively the maintainer of the app that is hosting Hypothesis (or at least, the integration of that app with Hypothesis).

Copy link
Member

@robertknight robertknight left a comment

Choose a reason for hiding this comment

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

If the feature flag is off when HTMLIntegration is instantiated, the feature remains disabled if the user logs in to a profile where the feature flag is enabled.

src/annotator/integrations/html.ts Outdated Show resolved Hide resolved
src/annotator/integrations/html.ts Outdated Show resolved Hide resolved
@acelaya acelaya merged commit df10f49 into main Jul 7, 2023
4 checks passed
@acelaya acelaya deleted the side-by-side-mode branch July 7, 2023 09:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants