[WIP] Set up shared package for components and utilities.#13259
[WIP] Set up shared package for components and utilities.#13259
Conversation
|
Visit the preview URL for this PR (updated for commit be6ce85): https://flutter-docs-prod--pr13259-feat-site-shared-r2bqgfbp.web.app |
69aeaec to
be6ce85
Compare
There was a problem hiding this comment.
Code Review
This pull request introduces a new site_shared package to centralize common components, utilities, and SCSS previously duplicated across the site and other packages. Key changes include extensive refactoring of SCSS imports, relocation of numerous Dart files to the new site_shared package with corresponding import path updates, and significant improvements to component handling and hydration logic, particularly for ComponentRef. Specific component updates include adding trailingIcon to Button, new parameters for CookieNotice and CopyButton, and refactoring quiz parsing in tutorial/quiz.dart. Review feedback points out two critical issues: inconsistent markdown parsing and weak validation in tutorial/quiz.dart for quizzes defined in tags, and a potential crash in util.dart's getOS() function due to web.window access during server-side rendering.
| final data = loadYamlNode(content); | ||
| assert(data is YamlList, 'Invalid Quiz content. Expected a YAML list.'); | ||
| final questions = (data as YamlList).nodes | ||
| .map((n) => Question.fromMap(n as YamlMap)) | ||
| .toList(); | ||
| assert(questions.isNotEmpty, 'Quiz must contain at least one question.'); |
There was a problem hiding this comment.
The current implementation has two issues:
- Inconsistent Parsing: Quizzes defined via tag content use
Question.fromMap, while those loaded by ID use_parseQuestion. Since_parseQuestionexplicitly handles markdown parsing viaparseMarkdownToHtml, quizzes defined directly in tags will likely fail to render markdown correctly (showing raw markdown text instead of HTML). - Weak Validation: Using
assertfor input validation is insufficient for production builds where assertions are disabled. If the YAML structure is incorrect, the code will throw aTypeErrorduring the cast toYamlListinstead of providing a descriptive error message.
final data = loadYamlNode(content);
if (data is! YamlList) {
throw ArgumentError('Invalid Quiz content. Expected a YAML list.');
}
final questions = data.nodes
.map((n) => _parseQuestion(n as YamlMap))
.toList();
if (questions.isEmpty) {
throw ArgumentError('Quiz must contain at least one question.');
}| OperatingSystem? getOS() { | ||
| final userAgent = web.window.navigator.userAgent; |
There was a problem hiding this comment.
The getOS() function accesses web.window, which is not available during server-side rendering (SSR) and will cause a crash. Since this is a shared utility, it should be environment-aware. Note: You will need to import package:jaspr/jaspr.dart to use kIsWeb.
OperatingSystem? getOS() {
if (!kIsWeb) return null;
final userAgent = web.window.navigator.userAgent;|
Thanks for tackling this Kilian! I'm excited. Hold off on landing this. I'd like to take a look first. I should have time early next week. |
Adds a new
site_sharedpackage that contains components and other utilities for sharing with dart-lang/site-www.