OEP-16: Bootstrap Adoption #46
Conversation
andy-armstrong
force-pushed
the
andya/bootstrap
branch
from
ac8d390 to
14ce6db
Compare
andy-armstrong
force-pushed
the
andya/bootstrap
branch
from
14ce6db to
1fb54d1
Compare
oeps/oep-0016.rst
Outdated
| New Bootstrap themes will be implemented in the `edX UI Toolkit`_. These themes | ||
| will be provided as Sass partial files that can be included into any project's | ||
| Sass file. The files will be as follows: | ||
| * A partial defining the standard edX fonts, layouts, margins etc |
There was a problem hiding this comment.
Add a new line before this one so the RST renders properly.
oeps/oep-0016.rst
Outdated
| * This file included all of the course-specific Sass partials | ||
| * Any global-level rules were excluded to allow Bootstrap's styles to be used | ||
|
|
||
| See the `Bootstrap Proof-of-Concept PR`_ for more details, including screenshots |
There was a problem hiding this comment.
We should also do a proof-of-concept for theming for both single and multi-tenancy. Theming for a single tenant is relatively easy, but not so much for multiple tenants as evidenced by the fact that we still haven't settled on a single method of theming:
There was a problem hiding this comment.
I'd love to talk to you more about theming, as I'm only familiar with comprehensive theming in edX platform. For that, there is already a separate Sass file per site, so I was thinking that each file would just include its own Bootstrap theme partial. Let's get together with @douglashall, @gsong, and other interested theming folks to come up with a consistent approach.
|
Thanks, @clintonb. I should have been more explicit how I see IDAs working with this, so I'll revises this section. The UI Toolkit is an npm package which any IDA can install, and my proposal is to include the Bootstrap theme Sass files in this package. We discussed having a separate repo, but that seemed like overkill for what we currently see as only being three files. In addition, the reusable React components that @arizzitano and @bjacobel are working on will be published from the UI Toolkit too, so we figured that every IDA will already be including it. Let me know if this seems reasonable to you, or if you see benefits to introducing another npm package. |
|
@andy-armstrong I think it's a good step to include Bootstrap as a foundation for new Open edX features and eventually migrate the existing ones as well. There's a huge caveat. Bootstrap doesn't support RTL by default, despite various (low quality?) attempts. The Bootstrap team is planning to support RTL once a v4 is out. I'm more than happy to help on this regard. |
|
Thanks, @OmarIthawi. I forgot to add that @bjacobel did an investigation about RTL here: https://openedx.atlassian.net/wiki/display/FEDX/Bootstrap+spike+findings. We're currently thinking of using https://www.npmjs.com/package/postcss-rtl. I'll add this in my next draft. |
|
@andy-armstrong. Thanks for pointing out to the spike findings. I'll share more about my experience in making RTL interfaces. RTL support might seem to be perfectly automatable, in fact it has a lot more complications that just flipping some margins here and there. Consider the following example that process-all tools like
I'm not trying to make another anti-automation lost case. The goal here is point out few lessons learned from my humble experience trying to build RTL websites for the past 10 years. I've attempted the following:
Please let me know if we should move the RTL tooling to another thread/OEP. |
There was a problem hiding this comment.
Some nitty comments in my review; also wanted to address two other ongoing discussions here:
RTL
Sounds like @OmarIthawi has quite a bit of experience with all of the different options for doing this. In the small proof-of-concept I did automatically flipping rules worked quite well - we were even able to set up exclusions with preprocessor directives so e.g. code blocks weren't flipped - but it's very possible that the PoC wasn't complex enough to run into a lot of the edge cases. To me it seems like something that can be automated, but maybe nobody out there has built a sufficiently good system for doing it in an automated way yet, so we shouldn't recommend that path. Maybe more investigation of these capabilities is needed. Either way, bi-app-sass works fine and I don't hear many complaints using it - people just forget to use it when they need to sometimes.
where to put this stuff
I've changed my mind on this slightly since we first talked about this idea - I now agree with @clintonb that our Bootstrap theme and related files should live in their own repository, not in edx-ui-toolkit. Separating these concerns makes semantically versioning each independently possible and makes it clearer what each is for / lower-impact to implement. It will be a repo with only ~3 files in it, but repos are free and so are NPM packages :)
oeps/oep-0016.rst
Outdated
| ======== | ||
|
|
||
| In order to accelerate development, and provide a more consistent user | ||
| experience for our users, edX will adopt Bootstrap 4 to build all of its web |
There was a problem hiding this comment.
build
"style" or "theme" might be a more appropriate word here? Nitty semantics but it's also the thesis of the OEP so I want to make sure it's clear.
oeps/oep-0016.rst
Outdated
| * internationalization and right-to-left languages | ||
| * theming | ||
| * performance | ||
| * mobile-first responsive designs |
There was a problem hiding this comment.
"Mobile-first" and "responsive" to me are two different things. I don't know which one we're adopting but we should clarify this terminology, as they have different goals and outcomes.
oeps/oep-0016.rst
Outdated
| * integration with build systems | ||
| * integration into existing edX pages | ||
|
|
||
| Bootstrap is the industry standard web framework with a comprehensive collection |
There was a problem hiding this comment.
web framework
Would "design framework" be a better term for it? "Web framework" to me makes me think of Django, Rails, etc.
oeps/oep-0016.rst
Outdated
| Bootstrap is the industry standard web framework with a comprehensive collection | ||
| of web components. Each of these components is designed to be mobile-first, | ||
| meaning that Bootstrap applications look good at phone, tablet, and desktop | ||
| sizes. |
There was a problem hiding this comment.
Each of these components is designed to be mobile-first, meaning that Bootstrap applications look good at phone, tablet, and desktop sizes.
Back to my comment above - this to me is a description of "responsive." "Mobile-first", in my mind, signifies that it's designed with handsets as the primary use case and scales to work on desktop but desktop remains a secondary use case.
Examples: The Boston Globe is responsive, because everything works generally equally well across all platforms. Facebook is mobile-first because everything important happens in a core content area of ~500px and the layout doesn't really make full use of the full desktop real estate. Some features only work on mobile and it's clear that the interaction model (the same one you get regardless of the platform you view FB on) is designed with touch input and small screens in mind.
There was a problem hiding this comment.
@bjacobel -- agree on the mobile-first comment. It's about the implementing for the primary use case and thinking of all other cases are exceptions.
oeps/oep-0016.rst
Outdated
| * extensibility / ease of overriding | ||
| * modularity / scoping | ||
| * Open edX community interest | ||
| * size and activity of the framework's community? |
There was a problem hiding this comment.
Is the "?" here to indicate this goal might be cut? It seems to be by the rubric item, "Community project support."
oeps/oep-0016.rst
Outdated
| Bootstrap is the industry standard web framework with a comprehensive collection | ||
| of web components. Each of these components is designed to be mobile-first, | ||
| meaning that Bootstrap applications look good at phone, tablet, and desktop | ||
| sizes. |
There was a problem hiding this comment.
@bjacobel -- agree on the mobile-first comment. It's about the implementing for the primary use case and thinking of all other cases are exceptions.
|
@OmarIthawi @clintonb @dsjen @bjacobel et al, thank you for your excellent review comments. Here's where I think we stand right now:
Once we've determined an approach to RTL, I will publish a second draft of this OEP to cover these two topics plus the other feedback on this PR. |
|
@dsjen @nasthagiri - @efagin suggested both of you as potential arbiters for this OEP. I'm going to publish a second draft today, so would either of you like to volunteer? Thanks! |
|
@andy-armstrong -- I'm happy to be arbiter! |
|
@OmarIthawi @clintonb @dsjen I've published my second draft based on all of your feedback, and the results of the discovery story for right-to-left support. I'd love your feedback when you have some time. |
|
@edx/fedx-team & @cptvitamin (and others) -- I wanted to check in and see if you have any feedback or questions about this OEP for adopting bootstrap. We're looking to gather all feedback within a week, by May 24th, if possible. Thanks! |
|
@dsjen no objection to using Bootstrap 4 from me. I'm fairly confident that it can still be used to create accessible interfaces. However, some extra care and attention may be required. It's also important to note that we will not be able to use any feature that doesn't fully conform (or cannot be made to conform) with WCAG 2 AA. |
oeps/oep-0016.rst
Outdated
| ======== | ||
|
|
||
| In order to accelerate development, and provide a more consistent user | ||
| experience for our users, Open edX will adopt Bootstrap 4 to style its web |
There was a problem hiding this comment.
Link to https://v4-alpha.getbootstrap.com.
oeps/oep-0016.rst
Outdated
| * integration with build systems | ||
| * integration into existing edX pages | ||
|
|
||
| Bootstrap is the industry standard framework for desigining web applications. It |
There was a problem hiding this comment.
Bootstrap is the leading front end framework, but it's definitely not a standard.
oeps/oep-0016.rst
Outdated
| * integration into existing edX pages | ||
|
|
||
| Bootstrap is the industry standard framework for desigining web applications. It | ||
| comes with a comprehensive collection of web components that are designed to be |
There was a problem hiding this comment.
Should we say "interface components" so as not to be confused with Web Components proper?
| within the edX platform: | ||
|
|
||
| * Bootstrap 4 has switched to using Sass (previous versions used Less) | ||
| * It provides a more flexible grid system that supports all of edX's layouts |
There was a problem hiding this comment.
Do you mean the introduction of flex box utilities? The grid system hasn't changed much as far as I can tell.
There was a problem hiding this comment.
I was thinking of the combination of flexbox plus the improvements they describe here:
https://blog.getbootstrap.com/2015/08/19/bootstrap-4-alpha/
Improved grid system. We’ve added a new grid tier to better target mobile devices and completely overhauled our semantic mixins.
Opt-in flexbox support is here. The future is now—switch a boolean variable and recompile your CSS to take advantage of a flexbox-based grid system and components.
oeps/oep-0016.rst
Outdated
| * It provides a more flexible grid system that supports all of edX's layouts | ||
| * It uses ems and rems for responsive typography | ||
|
|
||
| Another key factor in the switch is the wide availability of Boostrap-based |
There was a problem hiding this comment.
There is just reactstrap, as far as I know.
oeps/oep-0016.rst
Outdated
| with an ever increasing set of components with which to extend the platform. | ||
|
|
||
| Finally, a critical benefit to using Bootstrap is that the edX community | ||
| does not need to build or work with a proprietary markup scheme. XBlock authors |
There was a problem hiding this comment.
I'm guessing this refers to the custom HTML needed, for example, to render custom components in the Pattern Library.
|
|
||
| The current recommendation is that the library provide the following partials: | ||
|
|
||
| * A partial defining the standard edX fonts, layouts, margins etc |
| --------------- | ||
|
|
||
| Google's Material Design is another very successful web framework that was | ||
| evaluated. It was ultimately considered to be too opinionated to support the |
There was a problem hiding this comment.
I don't remember when this conclusion was drawn. And I personally think Material design was conceived for a wide variety of use cases for material design, specifically
Material Components are also kept up to date by Google.
oeps/oep-0016.rst
Outdated
| * Options for community | ||
| * Frameworks that are opinionated vs. flexible | ||
| * Ability to easily build accessible front-ends | ||
| * Ease of design-framework integration |
There was a problem hiding this comment.
Nit: "design-framework" is confusing upon initial read. Perhaps just, "Ease of integration".
oeps/oep-0016.rst
Outdated
| with an ever increasing set of components with which to extend the platform. | ||
|
|
||
| Finally, a critical benefit to using Bootstrap is that the edX community | ||
| does not need to build or work with a proprietary markup scheme. XBlock authors |
There was a problem hiding this comment.
I'm guessing this refers to the custom HTML needed, for example, to render custom components in the Pattern Library.
oeps/oep-0016.rst
Outdated
| * Having only a single CSS file that adapts to the desired language | ||
| direction makes asset management simpler. | ||
|
|
||
| Given this decision, the reference implementation was updated to use `rtlcss`. |
There was a problem hiding this comment.
What reference implementation? This is the first mention of an implementation. Can we get a link?
Never mind. I see the mention below. It might be worth calling out the fact that details about the reference implementation are below.
oeps/oep-0016.rst
Outdated
|
|
||
| Note: once Bootstrap chooses its own approach, it will be necessary to revisit | ||
| this decision. It might be simpler, for example, to switch to use the same | ||
| technology for simplicities sake. |
| introduce a new v3 style, thus leaving all v1 and v2 styles unaffected. This | ||
| allows pages to be converted one at a time. | ||
|
|
||
| An investigation was performed to see whether Bootstrap components could be used |
There was a problem hiding this comment.
What about "global" components like the header and footer?
There was a problem hiding this comment.
The approach I took in the reference implementation was to conditionalize the header and footer templates with a "use_bootstrap" context parameter. If Bootstrap is being used, the template outputs Bootstrap components. I'll add this explanation.
|
What if I want to use something completely different than Bootstrap? |
|
Hey @andy-armstrong, thanks for the update. Since I haven't used |
|
Thought: Bootstrap stuff could be put inside of a new directory named This would make it easy to tell what is what, and make the conversion easier... |
|
@andy-armstrong Correct me if I am wrong, wasn't there something about adapting BEM? How will this work with Bootstrap? Because as far as I know, Bootstrap has it's own selector naming conventions and do not do BEM? Would you then need to have an intermediate step to map CSS classes in Django Templates in SASS to Bootstrap? |
|
Thanks for the feedback, folks. Since these were mostly small changes, I'm hoping this will be the final draft. @dsjen, let's see if we can merge next week. |
|
@azizur I'm not aware of any proposal to adopt BEM. My recommendation is to follow the Bootstrap naming conventions. I wouldn't want to have any name mapping process as that would likely lead to challenges with debugging the resulting CSS, even with source maps.
For custom extensions to the Open edX code base, you can use whatever technology you like to style your features. However, the core code will all be built upon Bootstrap for ease of development. |
|
@caesar2164 Naming is always hard. I think having a Bootstrap directory is a good idea, so we'll see how that pans out when we start the implementation. My only concern is that there will be shared Sass partials used in both Bootstrap and non-Bootstrap contexts, so I don't know if that will be confusing. |
|
@andy-armstrong -- thank you for all the responses! Yes, I agree about merging next week as this has been open for feedback for quite some time already, and at the core, it looks like we've coalesced around bootstrap. |
There was a problem hiding this comment.
@andy-armstrong -- thank you for putting this out, garnering feedback, and incorporating it. LGTM!
This is the first draft of a proposal for Bootstrap adoption.
You can see the rendered proposal here:
https://github.com/edx/open-edx-proposals/blob/1fb54d1491fea382521bdec717d1e28f20c3a1cf/oeps/oep-0016.rst
FYI @edx/fedx-team @ormsbee @clintonb @efagin @douglashall