feat: Apps, routes, and slots #36
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There were several breaking changes, new warnings, and changes in here. - Migrate from @import to @use in SCSS files. - replace “compileType” with “mode” in css-loader options - Set “namedExport” to false to preserve behavior of earlier css-loader versions - Switching from ‘sass’ to ‘sass-embedded’ to improve build performance.
This commit upgrades us to the latest ESLint 9. It adds better linting for TypeScript, in particular. It removes eslint-plugin-airbnb and replaces it with the ‘recommended’ configs for eslint, typescript-eslint, react, and react-hooks. It also adds the eslint stylistic config, and modifies it to match our existing code style. Subsequent commits will fix all the styling issues found in this commit; the repository wasn’t linting very consistently before this, so somehow a bunch of stuff slipped through.
I had a checkout with a ton of linting changes and just overlooked these files.
Because of the new config, a ton of eslint ‘ignore’ comments and such are no longer necessary.
Also getting rid of some unneeded eslint disable line comments
Also using ‘exec’ on a Regex instead of match.
The existing createConfig helper is not appropriate for the new version of ESLint for two reasons - one, it uses webpack-merge, while we need to use tseslint.config to merge config. Second, the argument of a single “configFragment” object is not what tseslint needs - it needs an array of ConfigWithExtends objects. For these reasons, I just split the lint config helper out into a separate function.
PluralRules and RelativeTimeFormat are widely available in browsers today, so these are no longer necessary.
Modules that need these functions should implement them themselves - frontend-app-account in particular. We don’t want to load these lists for the vast majority of the frontend that doesn’t need them. This also removes the third party dependencies that backed the lists.
This simplifies configuration a bit; rather than an apps object with ids as keys, you just add apps and the app config itself contains the ID. Also adding a ‘federation’ sub-object to the FederatedAppConfig, just to make clear that those details are federation-specific.
This lets modules update a global ‘learning context’ containing a CourseInfo object. This is used by the header to display course information conditionally when a module needs it.
Helps to realize very little of it is ‘common’ - most is about APIs or specific frontend modules.
This is step one - the contents will now break. But I want to get this move in so that we can preserve history without git getting confused. If we also change the contents too much at the same time, it forgets that it’s the same file.
This rule suggests adding a ! to the end of a variable to assert that it is not null or undefined. For our nascent familiarity with Typescript, this seems too obscure to me. I’d rather see “variableName as ExpectedType” which feels much more explicit. Maybe someday when we’re all familiar with the ins and outs of TS.
There’s no JSX in this file.
- The “validateRequirements” functionality is no longer necessary - Typescript will throw an error if the types are incorrect. - Replacing use of PropTypes with TypeScript types and interfaces throughout. - Annotating functions with types. - Fixing a number of small type-related bugs and inconsistencies. - Removing types from @param doc comments. This is a choice I’m seeing online, since the type information is encoded in the actual parameters. Otherwise it’s just redundant information.
Need some new tests over here!
This commit is one of several signficantly refactoring the shell, header, footer, menu helpers, and site config into a system with frontend-plugin-framework-style “Slots” as a central concept. An “App” consists of a set of routes, slot configurations, and i18n messages. Routes are react-router routes, and are generally unrelated to slots. Slots are configured via operations. The only type of slot right now is a “widget” slot, which renders UI components into a ‘layout’ container. There are operations to work with widgets and the parent slot layout. Subsequent commits will add new versions of the header/footer and menu helpers. It will then use them all in the dev-project and test-project.
This commit clears out a bunch of helpers for creating menus in the header and footer, simplifying them into a reusable component that’s aware of widget and route roles and can render itself as various link-like components. Subsequent commits to the header/footer sub-folders will use this (and the previous ‘slot’ commit) extensively.
This commit reworks the default header component to use slots for all its children. It supplies a default header app which makes use of all the slots in a way that should be a good starting point for most operators.
Like the previous commit did for the header, this commit refactors the footer to use slots for configuration.
This refactors site.config.dev.shell and the sample pages in dev-project to use the new system. The ‘home’ and ‘user’ apps in dev-project export App configurations with routes and slots to test out the various features of the new system.
We’ll add these back in once they’re actually usable.
openedx-webhooks
added
the
open-source-contribution
|
Thanks for the pull request, @davidjoy! What's next?Please work through the following steps to get your changes ready for engineering review: 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. 🔘 Let us know that your PR is ready for review:Who will review my changes?This repository is currently maintained by Where can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:
When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
|
This was supposed to be a pull request to my own |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Wrong target branch for this PR, closing.