feat!: use provides for chromeless shell mode#220
Conversation
72bd2f9 to
e50de2b
Compare
| function isActive(providesKey: string): boolean { | ||
| const activeRoles = getActiveRoles(); | ||
| const inactiveRoles = getProvidesAsRoles(providesKey); | ||
| return !inactiveRoles.some(role => activeRoles.includes(role)); | ||
| } |
There was a problem hiding this comment.
It took me a bit of time to wrap my head around this. After digging into getProvidesAsRoles and isSlotOperationConditionSatisfied I think I know why it threw me for a loop: inactiveRoles sounds too much like activeRoles. It's a bit strange that activeRoles means "all the roles that are currently active" and inactiveRoles means "roles where we should hide the header/footer."
The most readable thing that came to my mind was to just split it into 2 functions
function showHeader(): boolean {
const activeRoles = getActiveRoles();
const hideHeaderRoles = getProvidesAsRoles(hideHeaderRolesProvidesKey);
return !hideHeaderRoles.some(role => activeRoles.includes(role));
}
function showFooter(): boolean {
const activeRoles = getActiveRoles();
const hideFooterRoles = getProvidesAsRoles(hideFooterRolesProvidesKey);
return !hideFooterRoles.some(role => activeRoles.includes(role));
}but I could also see just renaming inactiveRoles, so something like
function isActive(providesKey: string): boolean {
const activeRoles = getActiveRoles();
const hideSomethingRoles = getProvidesAsRoles(providesKey);
return !hideSomethingRoles.some(role => activeRoles.includes(role));
}but I can't think of a good name for "something" in there.
There was a problem hiding this comment.
Yeah, I'm with you. Lemme see what I can come up with.
There was a problem hiding this comment.
Went with hideRoles. Simple and more semantic.
But I didn't stop there. I also realized isActive is about the widget, not any particular role, so renamed that too.
But I didn't stop there, and many renames along the way, I realized provides: is not a very good name for what we're doing with it, which is providing configuration for a feature some other app actually provides. So, it's now "features:", the identifiers are featureIds, and the actual strings are org.openedx.frontend.feature.aDescriptiveVerb.v1.
a53e36a to
1f74a3b
Compare
aba3803 to
d3463fb
Compare
brian-smith-tcril
left a comment
There was a problem hiding this comment.
I'm trying to think through how to make this functionality understandable. In our earlier slack conversations (back when it was provides) I mentioned
I'd like the keys to represent what is being provided more than the intended consumer. If people use existing ecosystem ids then I'd expect to see the pattern be "provide everything the one consumer I'm thinking about right now needs with this key" instead of "provide data with meaningful keys so consumers can selectively choose what they need to grab from provides"
With features (and specifically feature ids like org.openedx.frontend.feature.hideHeader.v1, that is definitely not the case. The "hide header" feature is very much targeted at one specific consumer (the APPEND operation for the Header component in org.openedx.frontend.slot.header.main.v1.
That being said, we have a very real need for apps to be able to send data/configuration to other apps, and consume data/configuration from other apps.
At this point I think I prefer provides to features, but I'd want the keys to be:
org.openedx.frontend.provides.courseNavigationRoles.v1- Any app can look for
courseNavigationRolesand see what roles are related to course navigation. It's not coupled to the navigation bar.
- Any app can look for
org.openedx.frontend.provides.fullPageRoles.v1- Any app can look for
fullPageRolesand see what roles are rendered without the header and footer. Maybe someone has a privacy notice app that they generally append toorg.openedx.frontend.widget.footer.desktopCopyrightNotice.v1in the footer but they also need on pages that are rendered without a footer, so they want to make sure to handle it appropriately when the footer isn't rendered. - We could also do
org.openedx.frontend.provides.headerlessRoles.v1andorg.openedx.frontend.provides.footerlessRoles.v1if combining the two is a problem, but I thinkfullPageRolesis clearer.
- Any app can look for
With that in place, shell/app/ts could have something like
function showHeaderAndFooter(): boolean {
const activeRoles = getActiveRoles();
const fullPageRoles = getProvidesDataAsStrings(fullPageRolesKey);
return !fullPageRoles.some(role => activeRoles.includes(role));
}I think overall I feel like there are 2 different directions to take this. Either we try to have apps provide data in ways that make sense for multiple consumers, or we lean into having apps just provide data/configuration for specific widgets that may exist elsewhere.
I'd prefer to go the structured data for multiple consumers route, but I'm very open to discussing.
| courseNavigationRoles: ['org.openedx.frontend.role.instructor'], | ||
| }, | ||
| features: { | ||
| 'org.openedx.frontend.feature.showCourseNavigationBar.v1': [ |
There was a problem hiding this comment.
I think this is meaningfully different from the previous example. The org.openedx.frontend.provides.courseNavigationRoles.v1 were used both to populate the links in the course navigation bar, and to determine if it should render for any given role. Changing this to just be showCourseNavigationBar makes it feel like it only does the latter.
Apps can now request chromeless mode (no header or footer) by
listing their route roles under providesChromelessRolesId in
their `provides` config. The shell checks these against active
roles at render time.
A new getProvidesAsStrings() runtime helper is added beside
getProvides(). The shell now exports providesChromelessRolesId
and providesCourseNavigationRolesId for use by consuming apps.
BREAKING CHANGE: The course navigation bar's provides data shape
is simplified from { courseNavigationRoles: string[] } to a
plain string[]. getProvidesAsRoles() is replaced by
getProvidesAsStrings() and moved from the shell into the runtime.
Co-Authored-By: Claude <noreply@anthropic.com>
d3463fb to
9292ea8
Compare
|
🎉 This PR is included in version 1.0.0-alpha.24 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Description
Apps can now request chromeless mode (no header or footer) by listing their route roles under
providesChromelessRolesIdin theirprovidesconfig. The shell checks these against active roles at render time.A new
getProvidesAsStrings()runtime helper is added besidegetProvides(). The shell now exportsprovidesChromelessRolesIdandprovidesCourseNavigationRolesIdfor use by consuming apps.The course navigation bar's provides data shape is simplified from
{ courseNavigationRoles: string[] }to a plainstring[], andgetProvidesAsRoles()is replaced bygetProvidesAsStrings()and moved from the shell into the runtime.Testing
The easiest way to test this is to run this PR in a frontend-app-authn workspace that also happens to be running the downstream PR:
openedx/frontend-app-authn#1663
If the header and footer don't show, it's all good.
LLM usage notice
Built with assistance from Claude.