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

Aspects should be optional #17

Closed
leggetter opened this Issue Oct 4, 2013 · 9 comments

Comments

Projects
None yet
4 participants
@leggetter
Contributor

leggetter commented Oct 4, 2013

As per the title - Aspects should be optional.

This means that the contents usually found in an aspect should be able to reside within the root of an application.

From an upcoming blog/tutorial about BRJS and Angular:


Create the Todo App

Use the create-app command to scaffold a BRJS application.

$ ./brjs create-app brjstodo

For now, applications need to reside within an apps folder within the BladeRunnerJS directory. The above command will create an application called brjstodo within the apps directory. That directory will have the following contents:

apps/brjstodo
├── app.conf    # Application configuration
├── index.html  # Application entry point
├── src         # For app-level JavaScript
├── resources   # For other app resources
├── themes      # CSS and images
├── libs        # Other app libraries

Does this provide enough as an acceptance criteria?

@thecapdan

This comment has been minimized.

Show comment
Hide comment
@thecapdan

thecapdan Aug 18, 2014

Contributor

#19

Contributor

thecapdan commented Aug 18, 2014

#19

@sospirited sospirited self-assigned this Aug 20, 2014

@sospirited

This comment has been minimized.

Show comment
Hide comment
@sospirited

sospirited Aug 20, 2014

Member

Initial exploratory testing looks good.

  1. straight copy-pasting of default-aspect into root level 'just works' (inc. unbundled-resources, themes etc)
  2. creating a war from a vanilla app with no aspect continues to load as expected

Will just review any acceptance-test level spectests we have just to ensure we got some coverage at a slightly higher level (from a user-perspective-level).

Member

sospirited commented Aug 20, 2014

Initial exploratory testing looks good.

  1. straight copy-pasting of default-aspect into root level 'just works' (inc. unbundled-resources, themes etc)
  2. creating a war from a vanilla app with no aspect continues to load as expected

Will just review any acceptance-test level spectests we have just to ensure we got some coverage at a slightly higher level (from a user-perspective-level).

@sospirited

This comment has been minimized.

Show comment
Hide comment
@sospirited

sospirited Aug 20, 2014

Member

@thecapdan pushed some specTests, will wait for you to have a quick glance at the coverage and see that you're happy with it (and/or aware of it!) before moving it across to Done.

This issue also needs an update to the release notes before we proceed to that aswell.

Member

sospirited commented Aug 20, 2014

@thecapdan pushed some specTests, will wait for you to have a quick glance at the coverage and see that you're happy with it (and/or aware of it!) before moving it across to Done.

This issue also needs an update to the release notes before we proceed to that aswell.

@sospirited

This comment has been minimized.

Show comment
Hide comment
@sospirited

sospirited Aug 20, 2014

Member

@andyberry88 So what is the acceptance criteria regarding newly created apps? They currently create the default-aspect folder instead of creating the aspect assets at the root level. Does this need to be changed?
CC @leggetter

Member

sospirited commented Aug 20, 2014

@andyberry88 So what is the acceptance criteria regarding newly created apps? They currently create the default-aspect folder instead of creating the aspect assets at the root level. Does this need to be changed?
CC @leggetter

sospirited added a commit that referenced this issue Aug 20, 2014

#17 Updating release notes for optional default-aspect
- will need further updateing to explicitly state to the user if we change the default create-app behaviour (whether or not it continues to create the 'default-aspect' folder)

@sospirited sospirited changed the title from Aspects should be optional to Default Aspect should be optional Aug 20, 2014

@leggetter leggetter changed the title from Default Aspect should be optional to Aspects should be optional Aug 20, 2014

@leggetter

This comment has been minimized.

Show comment
Hide comment
@leggetter

leggetter Aug 20, 2014

Contributor

@sospirited I've updated the subject of this issue and also the description to try to provide acceptance criteria. Is that enough information?

Or maybe the information you provided in this comment should also be part of the acceptance criteria?

Contributor

leggetter commented Aug 20, 2014

@sospirited I've updated the subject of this issue and also the description to try to provide acceptance criteria. Is that enough information?

Or maybe the information you provided in this comment should also be part of the acceptance criteria?

@sospirited

This comment has been minimized.

Show comment
Hide comment
@sospirited

sospirited Aug 21, 2014

Member

Latest fixes look good 👍 @andyberry88
All java tests with gradlew cleanjava testjava pass for me on Windows and already passing on Travis.

Exploratory testing went fine, as per previous comments. I've updated the release notes and tagged this issue so please have a quick glance and tweak the message we want to communicate to end users about this (cc @leggetter )

From my point of view, happy for this issue to be moved to Done (will move it now).

Member

sospirited commented Aug 21, 2014

Latest fixes look good 👍 @andyberry88
All java tests with gradlew cleanjava testjava pass for me on Windows and already passing on Travis.

Exploratory testing went fine, as per previous comments. I've updated the release notes and tagged this issue so please have a quick glance and tweak the message we want to communicate to end users about this (cc @leggetter )

From my point of view, happy for this issue to be moved to Done (will move it now).

@sospirited sospirited added 5 - Done and removed 4 - Test labels Aug 21, 2014

@leggetter

This comment has been minimized.

Show comment
Hide comment
@leggetter

leggetter Aug 21, 2014

Contributor

#917 leaks through to this issue because when you try to view the default aspect I'm seeing the error outlined in that issue.

Contributor

leggetter commented Aug 21, 2014

#917 leaks through to this issue because when you try to view the default aspect I'm seeing the error outlined in that issue.

@leggetter

This comment has been minimized.

Show comment
Hide comment
@leggetter

leggetter Aug 21, 2014

Contributor

@sospirited Since I'm not directly involved in releases could you please provide a pointer to where I can see the release notes you mention in #17 (comment) ? A link to the commit would be grand. Thanks!

Contributor

leggetter commented Aug 21, 2014

@sospirited Since I'm not directly involved in releases could you please provide a pointer to where I can see the release notes you mention in #17 (comment) ? A link to the commit would be grand. Thanks!

@sospirited

This comment has been minimized.

Show comment
Hide comment
@sospirited

sospirited Aug 21, 2014

Member

@leggetter fbbf8c7 link to the last release note update I did for this issue (just to capture SOMETHING as we move stuff to Done), expecting the wording to change after review(s)

Member

sospirited commented Aug 21, 2014

@leggetter fbbf8c7 link to the last release note update I did for this issue (just to capture SOMETHING as we move stuff to Done), expecting the wording to change after review(s)

sospirited added a commit that referenced this issue Aug 22, 2014

#17 Test variable name refactoring:
- renaming 'rootAspect' -> 'rootDefaultAspect'

@andyberry88 andyberry88 removed the 5 - Done label Sep 4, 2014

dchambers added a commit that referenced this issue Nov 11, 2014

Squashed 'brjs-sdk/workspace/sdk/libs/javascript/topiarist/' changes …
…from 07fce20..7355c55

7355c55 Merge pull request #17 from janhancic/gh-pages
aeb35a1 Optimized the most common use-case likely with Topiarist (verifying arguments using isA()) so that it's now ~2.5 times slower than doing the same check manually, rather than being ~25 times slower, as was the case before. To achive this I did have to relax the argument checking performed, but this actuallly resulted in the browser throwing a clearer error message than what Topiarist was producing anyway.
03e51ba Added a new command (npm run test-performance) that shows that topiarist is currently ~25 times slower than using typeof and instanceof directly for the most common use case -- verifiying argument types.
8d50258 Also skip `toString` as it can break in IE8
a703e66 Made exportTo() available as export(), to remain backwardly compatible with v0.0.3.
cc4b1d7 Prepare to make a new version of Topirarist available via 'npm', but also provide a small backwards compatiblity fix so we only have to update the minor version number.
4686e10 Updated all grunt dependencies to see if this fixes the builds on Travis, plus ensured all dependencies were against fixed versions, since we're not trying to run a continuous integration test against our upstream dependencies.
0687c68 adding more browser versions for testing incl safari

git-subtree-dir: brjs-sdk/workspace/sdk/libs/javascript/topiarist
git-subtree-split: 7355c5520530c3b441952d06b4673462a5894546

This issue was closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment