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

Simplify sample template #96

Open
wants to merge 1 commit into
base: sample-template
Choose a base branch
from

Conversation

cdebost
Copy link

@cdebost cdebost commented Apr 28, 2021

The straightforward bit:

  • Removes .editorconfig, .gitignore, LICENSE.md, README.md files. Not necessary in a sample.

For review:

  • Removes the assets/style/style.css from samples in favor of using the main app's stylesheet. I think it's much better to ensure some consistency in the global style rules and avoid surprises when moving from the sample to the actual app.
  • Removes the assets/icons directory, using favicon.ico as the icon instead. Also removes said icons from the manifest file and removes some responsive web app tags from index.html. Based on my understanding, we'd only need these icons to make samples into progressive web apps, which I don't think there would ever be a reason for.

@cdebost cdebost requested a review from tejaede April 28, 2021 01:35
@tejaede
Copy link

tejaede commented Apr 28, 2021

Removes the assets/style/style.css from samples in favor of using the main app's stylesheet. I think it's much better to ensure some consistency in the global style rules and avoid surprises when moving from the sample to the actual app.

I agree that it's best to match the app's global style in the sample, but UI Kits don't have or need a global stylesheet so this results in a 404 and no global styles are applied. Maybe we can check for the existence of the main app stylesheet and only point to it if it exists? If not, I think it's better to leave it generic and make it the responsibility of the developer to match the sample styles to the app styles.

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.

2 participants