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

feat(core-styles): v2 #569

Merged
merged 173 commits into from
Jun 12, 2023
Merged

feat(core-styles): v2 #569

merged 173 commits into from
Jun 12, 2023

Conversation

wesleyboar
Copy link
Member

@wesleyboar wesleyboar commented Nov 16, 2022

Overview

  1. Use Core-Styles v2 via new setting, for future-compatibility.
  2. Use Core-Styles v0 by default, for backwards-compatibility.
  3. Freeze CMS v3.10.1 CMS stylesheets as site.___.css.
This solution avoids a CMS v4 right now. CMS upgrades would be more likely neglected if migration was required. (Even v3 CMS's upgrades had been neglected.) When 4.0.0 does come (to remove `TACC_CORE_STYLES_VERSION` setting), any CMS's not already upgraded to Core-Styles v2 obviously did not deserve the attention, and are allowed to remain stuck.

Related

Changes

Core CMS

  • changed core-styles dependency from v0 to v2
  • added TACC_CORE_STYLES_VERSION setting
    (so each cms can decide when it is ready to use newer core-styles)
  • added placeholder CDN stylesheets for core-styles bundles
    (not using https://unpkg.com cuz I worry that depending on an external service for primary stylesheets)
  • added templates that load Core-Styles v2 the way it is designed to be loaded
  • changed some stylesheets based on what now comes from Core-Styles
  • changed site.___.css stylesheets to be minified versions of what they were in v3.10.1
  • changed include of templates to be conditional based on TACC_CORE_STYLES_VERSION
  • changed loading of assets to be conditional based on TACC_CORE_STYLES_VERSION
  • changed demo to use new Core Styles v2 demo API
  • and more

Core Styles

Testing & UI

Future Compatibility

Backwards Compatibility

- fix(fonts): fp-1891 add regular italic, also add black (#71)
- feat(objects): warning about still-disabled o-site
- feat(components): icon updates from Core-Portal
- docs(trumps): comment about lifespan of u-empty
- chore(elements): body margin 0
- feat(dist): commit dist for clients relying on it (#70)
- chore: import dependencies in each pattern
In Core-Styles, icon CSS moved: trumps/icon → components cortal.icon.
J.R. and I agree a real CDN makes sense for Core-Styles, long-term.

J.R. doesn't know about this temp solution yet. He'll have better idea.
No need to re-build Core-Styles since we have it's `dist/`.

Related PR: TACC/Core-Styles#70
- Add core styles version setting.
- Load core-styles from pseudo-CDN.
- Use new CSS layer tech to load stylesheets.
Copy link
Member Author

@wesleyboar wesleyboar left a comment

Choose a reason for hiding this comment

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

Round 1 Review Comments

package.json Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
package.json Show resolved Hide resolved
Comment on lines +271 to +278
########################
# TACC: CORE STYLES
########################

# Only use integer numbers (not "v1", not "0.11.0"),
# so templates can load based on simple comparisons
TACC_CORE_STYLES_VERSION = 0

Copy link
Member Author

Choose a reason for hiding this comment

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

added TACC_CORE_STYLES_VERSION setting
(so each cms can decide when it is ready to use newer core-styles)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Eventually, will we be able to drop the if blocks for the version check, once all the portals are migrated forward? Or do you want to keep this setting to future-proof v3 development.

If retained, is the v1/v2 filename nomenclature the presence of the site. prefix in the v1 filenames (eg. 'site_cms/css/build/site.app.blog.css' vs 'site_cms/css/build/app.blog.css')?

Copy link
Member Author

Choose a reason for hiding this comment

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

Uncertain, the future of this setting is.

I want to remove it after migration, but we may be slow to migrate. Proof: CMD still has CMS v1 (pre Core-Styles) CSS like A2CPS and Frontera.

Yes, the site. prefix distinguishes CMS styles that use Core-Styles v0.

I do not like this, because it is not obvious. I think these site.____.css stylesheets belong in https://github.com/TACC/Core-CMS/tree/main/taccsite_cms/static/site_cms/css/src/_migrations. But Core-Portal and Frontera Tech Docs expect the filenames site.header.css and site.tacc-search-bar.css, and I have been too lazy to work in those repos again.

taccsite_cms/settings.py Outdated Show resolved Hide resolved
wesleyboar

This comment was marked as outdated.

@wesleyboar wesleyboar marked this pull request as ready for review May 26, 2023 16:50
@wesleyboar wesleyboar changed the title feat(core-styles): v2 ⚠️ feat(core-styles): v2 ❤️ May 26, 2023
@wesleyboar wesleyboar changed the title feat(core-styles): v2 ❤️ feat(core-styles): v2 ❗️ May 26, 2023
@wesleyboar wesleyboar changed the title feat(core-styles): v2 ❗️ feat(core-styles): v2 May 26, 2023
Copy link
Member Author

@wesleyboar wesleyboar left a comment

Choose a reason for hiding this comment

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

Round 2 Review Comments

taccsite_cms/static/site_cms/css/.postcssrc.yml Outdated Show resolved Hide resolved
bin/build-css.js Outdated Show resolved Hide resolved
taccsite_cms/templates/style_guide.html Outdated Show resolved Hide resolved
taccsite_ui/fractal.config.js Show resolved Hide resolved
taccsite_ui/fractal.theme.js Outdated Show resolved Hide resolved
taccsite_ui/patterns/core-cms Outdated Show resolved Hide resolved
taccsite_ui/patterns/core-styles Outdated Show resolved Hide resolved
taccsite_cms/settings.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@taoteg taoteg left a comment

Choose a reason for hiding this comment

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

Had a few inquiries and minor comments, but LGTM!

Copy link
Member Author

@wesleyboar wesleyboar left a comment

Choose a reason for hiding this comment

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

Peer Review Feedback

  1. Move site.___.css into _migrations.
    feat(core-styles): v2 - move v3.10 stylesheets to _migration #656
  2. (after merge) Ask CMD whether the conversion of a CMS-side can be tasked to an intern.
    I asked CMD privately; H.P. answers affrimatively.
  3. (after merge) Provide an architectural illustration.
    I will do this better, later, to more people. I can reference Shared UI - CSS - Architecture.
  4. (after merge) Determine how to make the CSS less daunting.
    This is a constant effort. After feat(core-styles): v2 - move v3.10 stylesheets to _migration #656, this PR will represent an improvement.

@wesleyboar
Copy link
Member Author

I moved the site.___.css to _migrations/v3-10_v3-11 via recently-merged #656.

@wesleyboar wesleyboar merged commit 5e32c8a into main Jun 12, 2023
@wesleyboar wesleyboar deleted the task/get-core-styles-beyond-0.11.0 branch June 12, 2023 23:52
@wesleyboar wesleyboar restored the task/get-core-styles-beyond-0.11.0 branch June 12, 2023 23:54
wesleyboar added a commit to TACC/Core-CMS-Custom that referenced this pull request Jun 21, 2023
@tacc/core-styles v2+ does not limit input field length.

But APCD relies on it (the limit was built for APCD's first form.)

This commit need not be in main until APCD loads Core-CMS post #569.

TACC/Core-CMS#569
@wesleyboar wesleyboar deleted the task/get-core-styles-beyond-0.11.0 branch November 13, 2023 21:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature A new feature or replacement of present feature priority ▲▲ Very high priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants