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

OEP-0023: Style Customization (Draft) #64

Merged
merged 1 commit into from
Aug 6, 2018
Merged

OEP-0023: Style Customization (Draft) #64

merged 1 commit into from
Aug 6, 2018

Conversation

arizzitano
Copy link
Contributor

No description provided.

announce official support for the new system.

8. Removal of legacy theming-related code may begin two named releases
after the announcement of its deprecation.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd be in favor of a more aggressive deprecation timeline for the oldest theming technologies, namely Stanford theming and microsites. As far as I know, we (OpenCraft) converted all our client themes to comprehensive theming and Sites framework several named releases ago, and I assume that many others would have done the same. We're somewhat used to needing to fix themes with every named release anyways, so I don't think converting from stanford theming to comprehensive theming (for example) is a notably heavier lift than the typical sort of work that needs to be done during an upgrade. We'll see what others say though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bradenmacdonald we would love nothing more than to deprecate existing systems ASAP -- plus, I think it would create some motivation to ramp up adoption of proposed new systems. What do you think about announcing legacy deprecation effective with the alpha release of the new style theming system, and then removing legacy theming code starting with the next sequential named release?

Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about announcing legacy deprecation effective with the alpha release of the new style theming system, and then removing legacy theming code starting with the next sequential named release?

@arizzitano I think that would be great! But it's not me you'd need to convince... I think we'd need more input from the community to see if anyone else objects strongly to that plan.

supported within the scope of this theming system. A system to
enable content modification may eventually be developed and
supported, but its definition is outside the scope of this
proposal.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this because with the new frontend technologies that are being introduced, theming that includes content modification is more technically challenging to implement? If so, I believe that's all the more reason to think about that now and lay a good foundation for it, rather than deferring it and having to kludge it together later.

The existing theming systems support content modification, so a new theming system that doesn't will actually be removing capabilities from the platform. This decision could significantly reduce uptake, as anyone whose legacy theme currently includes content modification would be unlikely or unable to convert their theme to the new system.

I would prefer to see "content modification" treated as a requirement that the new theming system must support before all of the legacy systems can be officially deprecated. (But as I commented elsewhere I still think most of the legacy theming systems other than comp. theming should be deprecated ASAP regardless.)

I've put some technical suggestions in a gist at https://gist.github.com/bradenmacdonald/34bf8bfc8f737f9b23a2664091b8f115 if that helps.

Copy link

Choose a reason for hiding this comment

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

Content modification is a very common requirement, so I agree with Braden it should be clear how that's going to be supported before the frontend starts being rewritten in React. While content modification with current comprehensive theming system is often kind of messy, modifying a React based interface is going to be even worse if it's not clearly thought out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bradenmacdonald I think this is an excellent approach and would be interested in seeing it grow into an OEP! The deprecation path is a broader conversation -- I'll leave a more general comment below.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is an excellent approach and would be interested in seeing it grow into an OEP!

@arizzitano Great! Do you know if you or anyone at edX is interested in taking that on? I might be able to find someone to do so but I don't have capacity to develop it further myself right at the moment.

CC @mtyaka in case you're interested :)

2. The core contributor team will develop an example theme compliant
with the new system and use it to build support into the platform.
Existing work [8] on such a theme already exists in the
edx-bootstrap repository.
Copy link
Contributor

Choose a reason for hiding this comment

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

Will edX.org be using a theme as well, that's notably different from the default Open edX (lack of) theme?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes! A Bootstrap theme currently exists for edx.org: https://github.com/edx/edx-bootstrap/tree/master/sass/edx

============

- All existing theming systems will be deprecated and support will
eventually be removed from the platform.
Copy link

Choose a reason for hiding this comment

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

I'm in favour of removing obsolete systems, but we need to have way to do content modification first.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Definitely agree! My wording around some of this was unclear -- I'll leave a comment below, and clean up the language.

frontend architecture.

- The Open edX platform’s static asset size and build times will be
greatly reduced.
Copy link

@mtyaka mtyaka Jun 20, 2018

Choose a reason for hiding this comment

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

Would this enable real-time theming? It would be great if you could set some color values in the admin, click a button and immediately see the changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Real-time theming comes up a lot, and while we agree it's ideal from an admin/maintainer experience standpoint, the technical complexity involved in enabling it for new and legacy systems is not a burden we currently have the resources to shoulder. CSS variables are promising, but would end support for IE11; CSS-in-JS is another (controversial!) option, but will not work with legacy non-JS UIs. A while back I even looked into building an in-browser SASS compiler, but was unable to make much progress.

Like the current style theming system, this approach is SASS-based and will need to compile first; however, it involves a single scoped set of variables (typically in one file). We would certainly be open to implementing a real-time theming system in the future -- technical suggestions are welcome -- but I don't think it needs to be a blocker for approving Bootstrap adoption.

Copy link

Choose a reason for hiding this comment

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

@arizzitano Definitely not a blocker for Bootstrap adoption, but great to know that real-time theming has been under consideration!

At OpenCraft we did a short discovery on how a (near) real-time theming could work. It would be great if you could give it a quick look to confirm whether the proposed implementation makes sense.

frontend architecture.

- The Open edX platform’s static asset size and build times will be
greatly reduced.

Choose a reason for hiding this comment

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

Great to see this kicking off! I agree we need a single system that would be properly documented and maintained, however using only Bootstrap theming (only visual part) is, in my opinion, not going to be sufficient for theming needs that vendors and maintainers of instances have. There are a lot of situations that require changing the content of pages, with one of the most obvious ones being setting the contents of static pages and changing the header and footer menu items (those are just the lightest modifications that come to mind, there are a lot more).

I don't see Comprehensive Theming as a system that doesn't cover a lot of needs - the template overriding system is something we definitely need and use. The problem lies more in usage of different types of templates (underscore) and lack of a proper API/documentation on how to include certain elements of the platform into custom templates. So doing customisations and implementing our custom templates is not a process where we refer to docs, but we rather figure out how everything works in the default templates and replicate it in our own. So when further work is done on the frontend by the core team and released in the next named release, a lot of stuff breaks because, naturally, the core team doesn't know what we use and what we don't (and I really wouldn't expect they do, after all we're essentially hacking stuff all around in our work).

What would certainly be helpful is a system where templates are clear and can be overridden (in a way we can override them using Comprehensive theming), where it's very clear and documented how to implement the LMS features in custom templates (for example: You can call the list of 5 newest courses on the platform using X, passing these arguments and expecting this response), a centralised place where default SASS variables can be overridden in the theme and documented classes that can be reused (I guess Bootstrap would answer this). A prerequisite for this would, I guess, be a cleanup of existing templates in the platform.

My question would be - what is the relation between this and the planned effort of complete separation of frontend and backend? Regarding how closely related to those future plans and the future architecture the new theming engine is, Is it worthwhile to scrap everything and build a new theming engine from scratch, or maybe just improve the Comprehensive theming engine as an intermediate solution before the full transition to a separated frontend?

I have also started a project to explore the possibilities of a future completely separated LMS (including the caveats, and "what do we need to make it work"): https://github.com/appsembler/react-lms
It is in a very early stage, however as you can see I have addressed theming in it in a very similar fashion to current Comprehensive theming - maybe/hopefully it helps in your future explorations for the platform.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@grozdanowski excellent points about the utility of comp theming template overrides -- as others have raised this as well, I'm addressing below in a broader comment. As much as I'd love to scrap the legacy systems and concentrate on building the new system instead, internal resourcing constraints will mean this needs to roll out gradually, rather than an all-in-one rebuild.

I envisioned the new system with the primary intention of working with the newer (decoupled frontend) architecture pattern. Achieving compatibility with legacy code may involve some hacky one-off integrations, but since the goal is to eliminate legacy frontends anyway, supporting them is less of a higher-order concern.

Your React LMS has actually been a major rallying point for us internally -- especially considering the timeline within which you completed it! We will definitely check out your theming system as well; looking forward to gathering insight and inspiration from your approach.

@arizzitano
Copy link
Contributor Author

@bradenmacdonald @mtyaka @grozdanowski To clarify some of the language around deprecation, we have no plans to deprecate existing systems entirely without existing support for content overrides. Our plan was: introduce Bootstrap theming system for style theming -> deprecate legacy systems for style theming only (i.e. remove support for SASS overrides); introduce React module override system for content modification -> deprecate legacy theming systems entirely. We will never remove support for comp theming until suitable replacement(s) for all its capabilities exist.

My intent in excluding content modification from this OEP was to keep the scope of this OEP manageable, enable smoother adoption of the Bootstrap theming system, and avoid yoking Bootstrap theming approval to a React override system -- which, implemention-wise, is essentially entirely separate. I still want to keep this OEP focused on styles and am cleaning up wording accordingly.

That said, I'd like to start planning for an override system to handle the content modification capabilities of existing theming systems. Braden's proposal for a React-based module override system is an excellent start and would be a good jumping-off point for a follow-on OEP.

I am currently tightening up some of the language in this OEP to clarify deprecation intent. If anyone has questions or comments specific to style theming or Bootstrap adoption, please raise them!

@bradenmacdonald
Copy link
Contributor

Thanks for clarifying, @arizzitano ! Sounds great, and we're excited to see this moving ahead.

@nasthagiri nasthagiri changed the title OEP-0023: Theming OEP-0023: Style Customization Aug 6, 2018
@nasthagiri
Copy link
Contributor

After checking in with @bill-filler, the arbiter, we are merging this OEP while keeping it in Draft state, until the time comes for a team to build a reference implementation and a plan forward.

@nasthagiri nasthagiri changed the title OEP-0023: Style Customization OEP-0023: Style Customization (Draft) Aug 6, 2018
@nasthagiri nasthagiri merged commit da07b0e into master Aug 6, 2018
@nasthagiri nasthagiri deleted the ari/theming branch August 6, 2018 14:56
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