-
Notifications
You must be signed in to change notification settings - Fork 86
Conversation
📊 Bundle Size Report
|
docs/articles/Getting-Started.md
Outdated
```bash | ||
export NODE_ENV=development | ||
|
||
npx -p yo -p @americanexpress/generator-one-app-module \\ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why break this onto a separate line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a pretty long command and breaks flex positioning in mobile devices.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 but when it gets put to the docs it isn't a simple copy and paste
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly, breaking it up like this is a little confusing. I suspect many would think these 2 lines are separate commands
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If putting it on one line would make it less confusing, I'm all for it. Will update
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in 789f657
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good start, i think we might want to focus on the micro-front end part then reference the recipes for everything else such as styling.
docs/articles/Getting-Started.md
Outdated
### Generating a Module | ||
|
||
The first step to get started with One App is to generate a Holocron module. | ||
There are two different Holocron module types, root and child module. Let us |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should explain what the difference is between root and child module
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'd love if we could add images to describe this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can plan to add images later, I agree would be so helpful!
docs/articles/Getting-Started.md
Outdated
When `one-app-runner` is fully loaded and running, we can navigate to `http://localhost:3000` | ||
and view our Holocron module in the browser. In another terminal window you can run | ||
|
||
```bash | ||
npm run watch:build |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When `one-app-runner` is fully loaded and running, we can navigate to `http://localhost:3000` | |
and view our Holocron module in the browser. In another terminal window you can run | |
```bash | |
npm run watch:build | |
When `one-app-runner` is fully loaded and running, we can navigate to `http://localhost:3000` | |
and view our Holocron module in the browser. | |
```suggestion | |
> It can take a few minutes when `one-app-runner` starts for the first time. |
Open another terminal window and run:
npm run watch:build
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good. suggestion, I will be adding this manually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated 789f657
docs/articles/Getting-Started.md
Outdated
|
||
```bash | ||
npm run build | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
```bash | |
npm run build | |
``` | |
You can use also build the module outside of watch mode:` | |
```bash | |
npm run build |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will add this manually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added these
docs/articles/Getting-Started.md
Outdated
## 📖 Table of Contents | ||
* [Generating a Module](#generating-a-module) | ||
* [Running One App](#running-one-app) | ||
* [Adding CSS Styles](#adding-css-styles) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i really like how you've broken this down into different sections
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
docs/articles/Getting-Started.md
Outdated
* [Creating Routes](#creating-routes) | ||
* [Module State and Data](#module-state-and-data) | ||
* [Configuring One App](#configuring-one-app) | ||
* [Development](#development) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should be more specific about this, what do you think? Setting up your development environment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I totally agree that this section is bare compared to the rest, I'll update it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
docs/articles/Getting-Started.md
Outdated
npm run build | ||
``` | ||
|
||
When ready to publish to production, make sure to set `NODE_ENV=production` before building. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we mention why here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this should be in a blockquote
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good points, I will update
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
docs/articles/Getting-Started.md
Outdated
"name": "my-module", | ||
"one-amex": { | ||
"bundler": { | ||
"webpackConfigPath": "webpack.config.js" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we recommend webpackConfigPath
for the first setup
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nellyk Agree.. a custom webpack config is usually a feature for advanced users and not necessarily required when you are just getting started
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point on that, I wanted readers to know that it is possible, however as you both mentioned, it is an advanced feature.
docs/articles/Getting-Started.md
Outdated
"name": "my-module", | ||
"one-amex": { | ||
"bundler": { | ||
"webpackConfigPath": "webpack.config.js" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nellyk Agree.. a custom webpack config is usually a feature for advanced users and not necessarily required when you are just getting started
docs/articles/Getting-Started.md
Outdated
> | ||
> [Adding Styles Recipe](../recipes/adding-styles) | ||
|
||
### Creating Routes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should adjust the order of this section to be before adding styles... it would be ideal if you could continue from the previous step, then show developers how to add a second module locally and load it using moduleRoute
so it keeps the flow
Additionally we could also show how to load modules with RenderModule
from holocron
that way we have covered all the possible scenarios for module composition 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually did want to talk about RenderModule
! The reason I left it out was the size of the article was growing... Inthe future, we should think about making a recipe on the different ways to render a module.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
About ordering sections, I get where you are coming from when switching the two sections.. the reason I'd want to keep the CSS as the first activity in the module is the ease to set up, a core part of web development (CSS) and direct visual feedback will give readers (especially new to One App or JavaScript) a soft start for the first thing they do in a module. I feel that would segway into more JavaScript heavy sections that follow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree with your take @Francois-Esquire since users can still load the webpage from the root, afterwards they can look at adding routes
docs/articles/Getting-Started.md
Outdated
|
||
We can import the stylesheet into our module and use CSS modules to access the class name | ||
for each selector by its name. The class names are unique when they are generated which | ||
avoids unwanted cascading of styles in our document. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could expand a bit more here to explain that the reason we use cssModules is because modules should be independent and we don't want css from other modules to affect them... hence the unique class name / avoiding collision
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will add a bit to this sentence
docs/articles/Getting-Started.md
Outdated
); | ||
} | ||
|
||
export default function MyModule({ moduleState = {}, moduleLoadStatus = 'loading' }) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should recommend moduleLoadStatus
to check for dataLoading events, after our discussion on on loadModuleData
it seems like there is no correlation between the loadModuleStatus
and the "Data loading status" we should double check that is the case before suggesting this pattern in our docs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i agree on this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair point, will update this
docs/articles/Getting-Started.md
Outdated
|
||
export default function MyModule() { | ||
const { theme } = useSelector( | ||
(state) => state.get('config'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe consider this example to load a single config value from the state? for best practices and performance we should recommend avoiding state.ToJS()
if possible
docs/articles/Getting-Started.md
Outdated
by creating our first Holocron module, then we will go through a few | ||
One App essentials to get up and running quickly. | ||
|
||
## 📖 Table of Contents |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have seen the CSP is a common pitfall...
What about adding a section for a basic CSP configuration at least explaining that one-app Blocks everything by default and that you won't be able to make any api calls unless you configure your CSP.
This should be explained as one of the main advantages of using one app as a best practice security feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with you however CSP is an advanced topic that deserves an article or a set of reference documentation on its own. Another concern with adding a section is it starts to make the document heavy for a getting started guide, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah we should add it to separate PR and maybe mention the article here once we have the CSP content
docs/articles/Getting-Started.md
Outdated
One App is a web application that combines together multiple experiences into a single, | ||
cohesive runtime. These experiences, or Holocron modules as we call them empower teams |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that "micro frontends" would make more sense here than "experiences". Is it really an accurate description to say Holocron modules are experiences? They are more versatile than that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with you, I'll update
docs/articles/Getting-Started.md
Outdated
```bash | ||
export NODE_ENV=development | ||
|
||
npx -p yo -p @americanexpress/generator-one-app-module \\ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly, breaking it up like this is a little confusing. I suspect many would think these 2 lines are separate commands
docs/articles/Getting-Started.md
Outdated
npm run build | ||
``` | ||
|
||
When ready to publish to production, make sure to set `NODE_ENV=production` before building. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this should be in a blockquote
docs/articles/Getting-Started.md
Outdated
One App has built in dynamic routing that uses each Holocron module to | ||
build out the router. There is a special property that we can assign to | ||
our module `Module.childRoutes` which would allow us to add routes to | ||
One App. `childRoutes` should be assigned a function that is given |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It only needs to be a function when you need to use the store for something, otherwise it can be an array or just a route with or without nested routes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TIL, thank you for sharing that 😄 I'll update the wording.
Co-authored-by: Ruben Casas <ruben@infoxication.net>
Co-authored-by: Jonny Adshead <JAdshead@users.noreply.github.com> Co-authored-by: Ruben Casas <ruben@infoxication.net>
Co-authored-by: Jonny Adshead <JAdshead@users.noreply.github.com>
Getting started guide for beginners. The article connects the dots between the ecosystem and the runtime.
Description
The article starts with creating a module and moves on to other topics with One App and Holocron.
Types of Changes
Checklist:
What is the Impact to Developers Using One App?
Documentation