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

Moved folder structure #268

Merged
merged 21 commits into from Dec 13, 2014
Merged

Moved folder structure #268

merged 21 commits into from Dec 13, 2014

Conversation

seethroughdev
Copy link

This is a v2.0 milestone to create a more consistent directory structure for the project as described in: #262

There were no actual code changes, however, paths were updated to work with the new separate folders. In the demo files I removed a path to metricsgraphics-layout.css that has a 404 in production.

I also created specs, gulp, and scss folders. Those will be updated in future tickets.

I made a couple of small additions to the package.json to allow for future npm use. As well as an index.js file. That will be valuable if we can migrate the library to be commonjs/amd compatible.

Let me know if you want me to set the PR to a v2 branch or anything...

Hope it helps.

@seethroughdev
Copy link
Author

One thing I noticed is that the make.py has bootstrap scripts in the array and the gulpfile doesn't. So when i tested the current gulpfile, the distributed files are different. Not sure if this was intentional or not, but wanted to make sure to point it out.

Adam L and others added 10 commits December 11, 2014 17:16
…s/metrics-graphics into feature/structure-cleanup

* 'feature/structure-cleanup' of github.com:seethroughtrees/metrics-graphics:
  Moved folder structure
Moving file structure

Moved to demo
…s/metrics-graphics into feature/structure-cleanup

* 'feature/structure-cleanup' of github.com:seethroughtrees/metrics-graphics:
  Renamed to get dist at top of folder structure
  Reverting name changes
  Renamed folders to get dist at top
  Moved folder structure
  Moved folder structure
almossawi added a commit that referenced this pull request Dec 13, 2014
@almossawi
Copy link
Contributor

Wonderful, thank you. v2.0 is planned for release next Friday, Dec 18th.

@seethroughdev
Copy link
Author

No problem, if you decide to merge, I can rebase it again first.

I'd like to improve on the gulp build from: #222 -- it will make the other tickets we discussed easier to implement.

But i'll wait to see the status of this so I know where to setup the paths...

@almossawi
Copy link
Contributor

@seethroughtrees Yes, if you wouldn't mind rebasing, please, in preparation for merging it.

Adam L added 5 commits December 13, 2014 12:35
Moving file structure

Moved to demo

Renamed folders to get dist at top

Reverting name changes

Renamed to get dist at top of folder structure

Moved folder structure

Moving file structure

Moved to demo

Renamed folders to get dist at top

Reverting name changes

Moved folder structure

Moving file structure

Moved to demo

Renamed to get dist at top of folder structure
@seethroughdev
Copy link
Author

Hi @almossawi , okay, I fixed the merge conflicts, and re-updated all the paths for the new testem files. I was able to successfully run the new test suite with the updated directory structure to verify.

One thing I do notice is that the src/js files when compiled are still different than the /dist/ versions. I'm wondering if that's just how you guys are developing between versions? or if there's another issue here.

But regardless, the files should be good to go now. If you are going to go with this structure, I'd request testing and merging in sooner than later, as the conflicts are hard to manage with the new paths. And there are lots of improvements to be made after :)

Let me know. Thanks...

almossawi added a commit that referenced this pull request Dec 13, 2014
@almossawi almossawi merged commit 5dd3bb7 into metricsgraphics:master Dec 13, 2014
@almossawi
Copy link
Contributor

Hi Adam, Would you mind checking the paths in dev.htm and examples.htm, please. Also, seems like lib wasn't committed. Otherwise, I'll be back in a few hours and can take a look at it then. Thanks.

@seethroughdev
Copy link
Author

I moved lib to /dist/ so it would stay at the top of the structure. Looks like I forgot to update the html for that change. Doing it right now, it will be ready when you get back!

@seethroughdev
Copy link
Author

I updated the new PR here: https://github.com/mozilla/metrics-graphics/pull/272/files

One thing worth mentioning: to keep the /dist/ folders out of the examples folder but still able to reference them. An example server needs to point to root so the path is /examples/dev.htm instead of /dev.htm

Let me know if that makes sense and is acceptable. The other option is to duplicate the library files inside of /examples/ which would easily be automated with gulp on compile. So there still wouldn't be any maintenance.

option a: keeps a clear separation, but requires the extra folder in the pathname
option b: duplicates files, but as long as it is automated, it makes the example folder more self-contained

Let me know whatever you prefer. If you want option b, I can easily do it tonight.

almossawi added a commit that referenced this pull request Dec 13, 2014
@almossawi
Copy link
Contributor

I think it would be valuable to keep /examples self-contained. @hamilton Do you have a preference?

On another note, I wanted to fix the links to images/missing-data.png from /dist/metricsgraphics.css. Any issues with copying said image to /dist or would you suggest linking to its current location under /examples?

@seethroughdev
Copy link
Author

I agree with you on the self-contained. As long as the dist/ and examples/ versions are compiled from the same source, I think its no big deal. And only further decouples the two.

I have an updated gulp build to to PR in a bit, so I'll include pushing to both locations. I'll wait for @hamilton to weight in though.

As far as the missing-data.png, I've been thinking about that one as well. I guess if its truly part of the library, we should include it in /dist/, but it does feel slightly out of place to me.

If its really a must-have, I could also inline it with base64... Just a thought..

@almossawi
Copy link
Contributor

I suppose it wouldn't be unreasonable to move the .missing css rule to /examples/css/metricsgraphics-demo.css. That would be in line with some of the other recent changes that we've made. @hamilton Let me know if you disagree.

Thanks for the other PR 👍

@hamilton
Copy link
Collaborator

What if we re-implement the data-missing image as an svg? It would be more portable that way.

@hamilton
Copy link
Collaborator

Also would be pretty easy to do. It's only a couple of paths & areas.

@almossawi
Copy link
Contributor

@hamilton Sounds good. I'll create a new issue for that then.

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.

None yet

4 participants