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

Introduce ExtJS 5 example app #352

Merged
merged 5 commits into from
Aug 27, 2015
Merged

Conversation

chrismayer
Copy link
Contributor

This PR introduces an ExtJS 5 based example app.
The new example app is an ExtJS5-MVVM port of the existing MVC-example app. The port is not exactly one-by-one, but the output is very very similar to the MVC app.

The base structure has been created with Sencha Cmd and therefore it is compatible for the usage with Cmd. No compiled sources are included here because they can be easily generated with Sencha Cmd. So code redundancy is avoided.

At the moment the GeoExt package dependency is not resolved automatically. The developer has to clone the GeoExt project into the packages folder of the app on his local checkout of this example.

Steps after checking out this branch:

According to the discussion in #351 this seems not be final now, but I suggest to review this to have a start point we can work with.

@marcjansen
Copy link
Member

After I checkout the branch, what steps am I supposed to do to see the example? Is tehre some sencha command that I need to run? I am a total noob with regard to sencha, sorry.

Other remarks from the top of my head:

  • Can we have it just like we had it with the old app-example, so that there is a index.html loading the compiled file and a index-dev.html loading loads of single files?
  • examples.json needs to have an entry for this example

* "autoCreateViewport" property. That setting automatically applies the "viewport"
* plugin to promote that instance of this class to the body element.
*
* TODO - Replace this content of this view to suite the needs of your application.
Copy link
Member

Choose a reason for hiding this comment

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

⇧ Can be removed, right?

@marcjansen
Copy link
Member

I added a couple of other thoughts. Once I know how to look at this, I'll have another look. Thanks for the work so far on this, @chrismayer. I definitely want such an example in the library!

@chrismayer
Copy link
Contributor Author

Hi @bentrm and @marcjansen,
thanks for your reviews!
I tried to adress your remarks (where possible). Would be cool, if you could have another look.
I also added some steps to get the app running for dev purposes to the PR description.

  • Can we have it just like we had it with the old app-example, so that there is a index.html loading the compiled file and a index-dev.html loading loads of single files?

I am not sure if this is way to go with Sencha Cmd. It seems that you get your dev-setup by running sencha app watch, which starts you a webserver, which automatically restarts in case of changing the source files.

  • examples.json needs to have an entry for this example

I will go for this later on.

*/
"js": [
{
"path": "http://cdn.sencha.com/ext/gpl/5.1.0/build/ext-all-rtl-debug.js"
Copy link
Member

Choose a reason for hiding this comment

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

Is this a "right-to-left" build of Ext? I think this can be omitted for development all together an the microloader will pick up your local Ext files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My intention for this was to have solved the ExtJS dependency by a CDN resource, like it is done in the other examples. So you do not need a local copy of ExtJS.
You are right, this is a "right-to-left" build. This surely can be changed to the normal debug version of Ext.

@bentrm
Copy link
Member

bentrm commented Apr 28, 2015

I added some comments inline. Having a look at your work, I added support to build the app without recursively cloning GeoExt into the examples packages folder. Would you be ok with me sending a PR to your working branch?

@marcjansen
Copy link
Member

Just cherry picking such a commit would probably be easier and would result in a nicer history. Just my two cents.

@chrismayer
Copy link
Contributor Author

@bentrm thanks a lot for your ongoing effort. Feel free to modify the branch. As @marcjansen suggested maybe cherry-picking would be a good option.

@chrismayer chrismayer mentioned this pull request May 12, 2015
9 tasks
@marcjansen
Copy link
Member

WRT to cherry picking: simply publish your commits somewhere @bentrm, @chrismayer can then cherry-pick them if he likes them.

@bentrm
Copy link
Member

bentrm commented May 19, 2015

Hi @chrismayer, @marcjansen, I just pushed to 4e9852d.
It should be possible to run sencha app watch and sencha app build inside of the app folder of the environment that you have described above without recursively cloning GeoExt into the example app folder.

I also removed the Sass stuff as it hasn't been used in the example app (setting skip.sass=1 in .sencha/app/sencha.cfg) and changed the build output directory to the GeoExt root folder (see app.json) adding temp file and directories to .gitignore.

@marcjansen
Copy link
Member

But this isn't a commit that builds on top of Chris' work, is it?

@bentrm
Copy link
Member

bentrm commented May 19, 2015

It is, it may have diverged though: branch log

@marcjansen
Copy link
Member

So the right diff is chrismayer:ext5-app...bentrm:ext5-app.

@chrismayer Any chance you can check it out?

@chrismayer
Copy link
Contributor Author

I merged the changes of @bentrm.
@bentrm & @marcjansen can you have another look at this? TIA.

@marcjansen
Copy link
Member

Good to be merged once the commit history is cleaned up. Thanks @chrismayer

chrismayer and others added 5 commits August 27, 2015 11:14
This introduces the base setup for an ExtJS 5 based example app.
The project structure is created with Sencha Cmd and the containing files
are only the raw code files. No compiled sources are included here.
The package dependency to GeoExt is also missing here to avoid redundancy.
This will be solved by a later commit or the steps to resolve the GeoExt
package dependency manually will be described in a README file.
This adresses the review notes of @marcjansen and @bentrm:
  - Remove unnecessary files
  - Use xtypes for chart and grid declarations
  - Several code cleanups
@chrismayer
Copy link
Contributor Author

I cleaned the commit history a bit. So now this is good to go, will merge now. Thanks to anyone who was involved.

chrismayer added a commit that referenced this pull request Aug 27, 2015
Introduce ExtJS 5 example app
@chrismayer chrismayer merged commit d10eaa2 into geoext:master Aug 27, 2015
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.

3 participants