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

Refactor Neat features as track types #889

Merged
merged 4 commits into from Jul 13, 2018

Conversation

Projects
None yet
4 participants
@cmdcolin
Copy link
Contributor

cmdcolin commented May 3, 2017

This is a reworking of NeatHTMLFeatures and NeatCanvasFeatures into track types

This could be considered a non-starter though because it creates the NeatHTMLFeatures and NeatCanvasFeatures as track types, but that could make integration with apollo difficult since apollo derives from HTMLFeatures, not NeatHTMLFeatures. Not sure if there is an easy workaround for that

This also has the side effect of fixing #651

@enuggetry

This comment has been minimized.

Copy link
Contributor

enuggetry commented May 4, 2017

Thanks.
I guess it's not as seamless, due the need for trackType in the track config.
I was hopeful it would solve #890, but guess it's not related. In fact, with this, bug #890 is also exhibited in NeatHTMLFeatures.

@cmdcolin

This comment has been minimized.

Copy link
Contributor Author

cmdcolin commented May 4, 2017

Both NeatCanvasFeatures and NeatHTMLFeatures with this PR have a "gradient" boolean config that can be enabled

@enuggetry

This comment has been minimized.

Copy link
Contributor

enuggetry commented May 4, 2017

Oh, it's default off. I see.

@nathandunn

This comment has been minimized.

Copy link
Contributor

nathandunn commented May 5, 2017

So, I think this is fine, in principle (making them first-class track types), though the existing implementations have a few bugs, which is why they are not not the default for Apollo:

https://github.com/GMOD/APOLLO/issues?utf8=%E2%9C%93&q=is%3Aissue%20is%3Aopen%20neatfeatures%20

I think we need to retain the plugin, though, as I don't think we are going to implement the track types in Apollo with those bugs. I run a lot of my sites with HTMLNeatFeatures on, and it looks cool, but it is hardly robust enough for production without some fixing.

@rbuels rbuels modified the milestone: 1.12.4 Jan 30, 2018

@nathandunn nathandunn changed the base branch from master to dev Feb 7, 2018

@cmdcolin cmdcolin force-pushed the track_type_neatfeatures branch from 1812f12 to bccd4f5 Mar 29, 2018

@rbuels rbuels added this to the 1.15.0 milestone Apr 20, 2018

@rbuels rbuels assigned cmdcolin and unassigned cmdcolin Jul 11, 2018

@cmdcolin cmdcolin force-pushed the track_type_neatfeatures branch from bccd4f5 to 372a87e Jul 12, 2018

cmdcolin added some commits Jul 12, 2018

@cmdcolin

This comment has been minimized.

Copy link
Contributor Author

cmdcolin commented Jul 12, 2018

I think this is probably going to go into 1.15.0! It does require the track type is explicitly declared to be NeatCanvasFeatures but I think there are a lot of wins with this route. I know there are some prominent neat features users so if anyone has feedback I'd be interested in knowing. @jogoodma

@nathandunn

This comment has been minimized.

Copy link
Contributor

nathandunn commented Jul 12, 2018

I agree. This is the right way to go on this and it will make maintenance of it easier.

@cmdcolin

This comment has been minimized.

Copy link
Contributor Author

cmdcolin commented Jul 13, 2018

There are two caveats to this PR which is that

  • gradients dont work on NeatHTMLFeatures, presumably due to trying to grab the style of the element before it is rendered to the dom
  • unannotated utr/outron not rendered in this code (canvas features has inferUTRs basically for this)

I imagine there can be a workaround to those issues but if those aren't roadblockers maybe this can still move forward

@rbuels rbuels merged commit cac8d8b into dev Jul 13, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@wafflebot wafflebot bot removed the in progress label Jul 13, 2018

@cmdcolin cmdcolin deleted the track_type_neatfeatures branch Jul 17, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.