search

publiclab / infragram

A minimal core of the Infragram.org project in JavaScript
https://infragram.org/sandbox/
GNU General Public License v2.0
44 stars 166 forks source link

Create version flags #438

Closed jywarren closed 2 years ago

jywarren commented 2 years ago

Fixes #0000

Make sure these boxes are checked before your pull request (PR) is ready to be reviewed and merged. Thanks!

gitpod-io[bot] commented 2 years ago

stephaniequintana commented 2 years ago

@jywarren From your comment here,

I started the flag implementation in https://github.com/publiclab/infragram/pull/438 - i will try to generate the dist files too -- but if that makes sense we can merge it and you can begin using if (options.version == 1) {} in your code!

This is great! Thank you for doing this. Question/thought:

@Forchapeatl, what do you think? Do you see any issues with merging this and including the flags into our code moving forward?

jywarren commented 2 years ago

I assume that with the flag in place we can get rid of the infragram2.js file and now operate soley on infragram.js. Is this correct/the path we are taking?

Yes, that should be right!

If so, should I open a PR that reflects this? (i.e. adding all of the version 2 functions from infragram2.js with the if (options.version == 2) {...} code to the infragram.js file and delete infragram2.js?)

You can, but we don't have to do it all in one pass. We can migrate anything in infragram2.js into infragram.js one step at a time, with the goal of a single file. But doing it all up front could be complex so there's not a huge rush.

For clarity, I see that you did not add the version flag code, ( options.version = options.version || 1;) to the individual src files. Do we need to include it in those individual files that we are adding only version 2 functions to as well?

Which files did you think it should be in? Maybe I missed something!

I'd very much like to merge this so that we can begin adding the flag to our code. I'd also like to update https://github.com/publiclab/infragram/pull/436 to include the flag.

Agreed, let's merge this as soon as i see what you mean in "For clarity, I see that you did not add the version flag code...", and then other work can be rebased on top. Thanks!

stephaniequintana commented 2 years ago

@jywarren,

We can migrate anything in infragram2.js into infragram.js one step at a time, with the goal of a single file. But doing it all up front could be complex so there's not a huge rush.

Ok, this sounds good - I also think its best to do this in smaller chunks. For now, index2.html (version 2) in connected to infragram2.js. I will leave this as is until we move everything over and simply add any new functions to both the infragram.js and the infragram2.js files

Can you please clarify; would you like us to utilize the version 2 flag on all functions that relate to version 2 or simply those that would break version 1? For example, #436 does not break version 1, (but also does not include the flag).

For clarity, I see that you did not add the version flag code...",

You added the flag to the src/Infragram.js file. I was just wondering if we needed to add it to the src/ui/interface.js file (and other src files that we are adding-to/changing). Infragram.js requires those modules, I just wasn't clear on whether that is sufficient. I actually think we're covered and can change anything later if need be 👍

jywarren commented 2 years ago

Oops, my apologies for missing this --

Can you please clarify; would you like us to utilize the version 2 flag on all functions that relate to version 2 or simply those that would break version 1? For example, https://github.com/publiclab/infragram/pull/436 does not break version 1, (but also does not include the flag).

I think only when necessary - that is, when not using the flag would break v1. Many additions simply address HTML bits that don't exist in v1, so they won't break anything if included. However, I think that it's good to mark such additions with a comment noting that they are for v2, so that we know what we can clean up later when we remove v1 code!

jywarren commented 2 years ago

You added the flag to the src/Infragram.js file. I was just wondering if we needed to add it to the src/ui/interface.js file (and other src files that we are adding-to/changing). Infragram.js requires those modules, I just wasn't clear on whether that is sufficient. I actually think we're covered and can change anything later if need be 👍

Well, once we include that flag in the main options parameter, it's accessible from most of the sub-files. See how we pass in the options object here:

https://github.com/publiclab/infragram/blob/86b2ddb3db34111b64b95db2b25cb41012f32a45/src/Infragram.js#L5

Then it's accessed within the file here:

https://github.com/publiclab/infragram/blob/86b2ddb3db34111b64b95db2b25cb41012f32a45/src/io/camera.js#L3

And you can see us using it here:

https://github.com/publiclab/infragram/blob/86b2ddb3db34111b64b95db2b25cb41012f32a45/src/io/camera.js#L111

Any subfile in /src/ which you see with the same options object passed through works in the same way, so that all options are consolidated. Does that make sense?

jywarren commented 2 years ago

Hi @Forchapeatl @stephaniequintana I just merged this - so you can now access it like: if (options.version == 2) in most files so you can switch between. Have you two been able to connect about how we'll aim to consolidate all functions in the same infragram.js file, and to use this flag to ensure v1 doesn't break when we add features that only work in v2? Thank you!

stephaniequintana commented 2 years ago

It does make sense. Thanks for clarifying 👍

stephaniequintana commented 2 years ago

We haven’t discussed this specifically yet.

With this merge, if we can include the flag on any outstanding and new PRs (where necessary), there should only be a few instances that we’ll need to address.

To help keep things organized, I propose that we do this after we merge @Forchapeatl’s latest work into version 2. I’ll touch base with @Forchapeatl and add this to our running thread.

jywarren commented 2 years ago

With this merge, if we can include the flag on any outstanding and new PRs (where necessary), there should only be a few instances that we’ll need to address.

Great!

To help keep things organized, I propose that we do this after we merge @Forchapeatl’s latest work into version 2. I’ll touch base with @Forchapeatl and add this to our running thread.

Sounds excellent.