Porting to D8 [#3010504]

Good morning
I am working on porting this module to D8.
I have pretty much success upto now (still to adapt path conditions)

Looking at the large portion of code relating to visibility, I am wondering if moving to the block system (make it a custom block to load the hypothesis embed.js on chosen pages only) would not be an option ?

Code replication/maintenance would be greatly reduced.

Has this been considered yet ? Would you see obstacles ?

Thanks

Comment	File	Size	Author
#4	hypothesis_porting_to_d8_as_block.patch	14.85 KB	f2boot
#3	hypothesis_porting_to_d8_as_block.patch	14.93 KB	f2boot
#2	hypothesis-porting_to_d8.patch	20.31 KB	f2boot

Comments

Comment #1

31 October 2018 at 11:00

f2boot created an issue. See original summary.

Comment #2

f2boot commented 1 November 2018 at 09:24

Status	File	Size
new	hypothesis-porting_to_d8.patch	20.31 KB

Here is a patch file porting hypothesis from 7.x-1.x to 8.x-1.x
Not tested thoroughly yet, but seems fine as a starting point

Moving now to working on a 8.x-2-x branch that would better use D8 functionalities (Block or Conditions system)

Comment #3

f2boot commented 1 November 2018 at 14:18

Status	File	Size
new	hypothesis_porting_to_d8_as_block.patch	14.93 KB

Here is a patch file porting drupal/hypothesis module from 7.x-1.x to 8.x-2.x (so it is an alternative to previous patch)

Hypothes.is client javascript is loaded in a (invisible) block.
The core of the initial module is preserved but all configurations are managed through the block system.

Comment #4

f2boot commented 2 November 2018 at 14:16

Status	File	Size
new	hypothesis_porting_to_d8_as_block.patch	14.85 KB

1 file was hidden/shown/deleted

Status	File	Size
hidden	hypothesis_porting_to_d8_as_block.patch	14.93 KB

Found two errors in #3 patch. Please use this one

@Maintainers, I have also made patches to restore "load hypothesis" permission and to allow user override showHighlights and openSidebar settings. I will add issues and upload patches for them if/when 8.x-2.x branch (or alike) is created.

Thanks

Comment #5

luke adams commented 8 November 2018 at 17:59

Hey f2boot, we appear to have done nearly the same exact thing! lol idk how I didn't catch this issue 4 days ago when I decided I needed a D8 port of this... anyway I've created an issue in the d.o contrib_tracker project and uploaded a zip of my D8 version of this.

https://www.drupal.org/project/contrib_tracker/issues/3012400

I've got my copy of this running in a dev site and behaving super nicely. I wonder if the block approach is best stuck into like a hypothesis_block module or something vs a total redo on how this module is setup and configured? It does seem to make sense, to just leverage how Drupal manages block visibility... could just add via the block management page or render said block via the theme.

Comment #6

f2boot commented 8 November 2018 at 20:45

Hi Luke,

Got a quick look to your code and looks like we got it very close indeed (I think yours is a bit cleaner ;) )
I have tried this "block" approach (#4) and I am using it on a dev site. At this point, I would say it is a better approach since it has much less code and we can use all context plugins to manage visibility.

I also have now a few other features on my dev site:
- add user prefs to use hypothesis or not (+ showHighlights and openSidebar)
- made a module derived from drupal/pdf to have hypothesis on pdf files
- working on a module to apply hypothes.is to html file in an iframe

I would love hypothesis code maintainer would open a 8.x branch so it is easier to share this.