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

Examples HLD #827

Closed
wants to merge 5 commits into from
Closed

Examples HLD #827

wants to merge 5 commits into from

Conversation

rajsite
Copy link
Member

@rajsite rajsite commented Nov 16, 2022

Pull Request

🀨 Rationale

HLD for the structure of nimble examples

πŸ‘©β€πŸ’» Implementation

See HLD

πŸ§ͺ Testing

N/a

βœ… Checklist

  • I have updated the project documentation to reflect my changes or determined no changes are needed.

specs/examples/README.md Outdated Show resolved Hide resolved
@rajsite rajsite requested a review from nate-ni November 16, 2022 01:21
specs/examples/README.md Outdated Show resolved Hide resolved
specs/examples/README.md Outdated Show resolved Hide resolved
specs/examples/README.md Outdated Show resolved Hide resolved
specs/examples/README.md Outdated Show resolved Hide resolved
specs/examples/README.md Outdated Show resolved Hide resolved
specs/examples/README.md Show resolved Hide resolved
@rajsite rajsite marked this pull request as draft December 9, 2022 22:35
specs/examples/README.md Outdated Show resolved Hide resolved
specs/examples/README.md Outdated Show resolved Hide resolved
specs/examples/README.md Outdated Show resolved Hide resolved
specs/examples/README.md Outdated Show resolved Hide resolved
@rajsite rajsite requested review from jattasNI, atmgrifter00, nate-ni and mollykreis and removed request for nate-ni May 17, 2023 22:37
@rajsite rajsite marked this pull request as ready for review May 17, 2023 22:38
@rajsite
Copy link
Member Author

rajsite commented May 17, 2023

@jattasNI @mollykreis @fredvisser @atmgrifter00 Y'all were reset as the HLD includes some significant changes such as a new npm package and breaking changes to Blazor and some existing site URLs that should be considered

- Alternative names:
- `@ni/nimble-bundled`
- `@ni/nimble-components-cdn`
- `@ni/nimbl-components-bundle`
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- `@ni/nimbl-components-bundle`
- `@ni/nimble-components-bundle`


Mixing them is very likely to break the application in hard to predict ways by creating multiple singletons in global scope, duplicating element registrations, etc. Moving them all to a separate package can eliminate possible confusion.
- Can inline all the related content, i.e. the needed nimble fonts can be copied from nimble-tokens into the cdn package making the package standalone. Doing that with the nimble-components package would not be appropriate.
- Proposed package name `@ni/nimble-bundle`
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't have a strong opinion, but I'd lean towards the alternative of @ni/nimble-components-bundle to make the package name better convey what is included. This isn't a bundle that includes all of nimble (Angular support + web component support + any/all other components we add first-class support for in the future).

- Examples for unsupported frameworks, i.e. example React app, vue.js app, etc. leveraging the web-component APIs directly. While we don't provide first class well-integrated support, having smoke test examples and quickly stackblitzable examples is handy.
- High-level component patterns - We could have demo pages showing making menu bar, application shell, etc stitching together nimble-components.

This might require more discussion as to the benefit of what is being illustrated. Alternatively we should be offering those higher-level patterns as supported libraries.
Copy link
Contributor

Choose a reason for hiding this comment

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

This statement seems to be a comment about the "High-level component patterns". Should it be a bullet point under that item?

- No explicit examples supporting vanilla JavaScript + CSS with local npm builds. Strongly encourage TS + SCSS.
- Examples will use [vite](https://vitejs.dev/)
- Supports TS + SCSS out of the box
- Web Chapter does not have an alternate recommendation yet for Vanilla apps
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- Web Chapter does not have an alternate recommendation yet for Vanilla apps
- Web UI Chapter does not have an alternate recommendation yet for Vanilla apps

- Examples should be specific and targeted
- Large examples or examples with multiple routes should be discouraged (but are not prohibited as to allow for examples demonstrating routing).
- Each example participates in the monorepo build
- Each example follows Web Chapter conventions:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- Each example follows Web Chapter conventions:
- Each example follows Web UI Chapter conventions:


Notes:
- A subfolder per `<framework>`, i.e.: `vanilla`, `cdn`, `angular`, `blazor`
- Examples should be specific and targeted
Copy link
Contributor

Choose a reason for hiding this comment

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

Will each example be totally standalone or would we have an Angular workspace containing an application per example?


Pros:
- Easier to open on mobile devices
- More reliable manual stress test environment (don't have to consider additional injected page content)
Copy link
Contributor

Choose a reason for hiding this comment

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

(don't have to consider additional injected page content)

Could you clarify what this means? Are you referring to stuff that Stackblitz adds to the page?

I have sometimes been annoyed when opening Stackblitz examples when they take more than a couple seconds to spin up their container. Having examples that avoid that would be nice. Obviously we have to consider the cost to implement it, but I like keeping it as part of the proposal.

- Avoids confusion within the Nimble Components package. Currently nothing prevents a user from accidentally mixing usage of the normal nimble component es modules, the all-components.js es module, or the all-components-bundle.js built output.

Mixing them is very likely to break the application in hard to predict ways by creating multiple singletons in global scope, duplicating element registrations, etc. Moving them all to a separate package can eliminate possible confusion.
- Can inline all the related content, i.e. the needed nimble fonts can be copied from nimble-tokens into the cdn package making the package standalone. Doing that with the nimble-components package would not be appropriate.
Copy link
Contributor

Choose a reason for hiding this comment

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

Some questions that might help me understand the vision for this package, which could help with naming and with deciding what is/isn't part of it.

  1. would apps ever have an npm dependency on it or just a runtime dependency on the build hosted on unpkg?
  2. would we publish a Nuget equivalent or just an npm pakcage?
  3. a client could use either nimble-bundle or a subset of the other packages, but never mix them, correct? So any assets an app might need will need to be a part of it: fonts, icons, CSS, tokens, components?

@rajsite rajsite marked this pull request as draft July 18, 2023 20:35
@@ -0,0 +1,157 @@
# Nimble Examples
Copy link
Member Author

@rajsite rajsite Jul 10, 2024

Choose a reason for hiding this comment

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

I'm gonna close this PR for now. Some things I think would be useful if re-opening:

  • Split out the new cdn / bundle packages as separate work
  • Consider completely narrowing the scope of the examples to just getting started examples. At which point maybe they should just create template packages:
  • Alternative: consider separate, 1 or more?, repos that have the getting started examples. Pros: might be easier to setup (create a repo with renovate). Cons: might not be as useful for quick one off examples locally (but probably fine for stackblitz).

@rajsite rajsite closed this Jul 10, 2024
@rajsite rajsite deleted the examples-hld branch July 10, 2024 19:21
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.

5 participants