Skip to content

Use a TailwindCSS managed class to centralize layout styling #6390

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

Merged
merged 1 commit into from
Oct 26, 2021

Conversation

danieldietrich
Copy link
Contributor

@danieldietrich danieldietrich commented Oct 25, 2021

Description

This PR performs a minimal change which would help a customer to integrate Gitpod Self Hosted more tightly. 🙏
In particular, this PR introduces a CSS class .app-container to solve the following aspects:

  • remove redundant style (good for Gitpod)
  • ability to integrate Gitpod in their product without the need of many customizations (good for Gitpod Self Hosted users) /cc @mrsimonemms

Please let me know if you prefer a different class name.

/assign @AlexTugarev

Related Issue(s)

How to test

Open this branch in a test environment and observe that the changed React components (see changed files) look like before (in the sense that the padding of the components is the same).

The overall risk of breaking something is nearly zero. If the styling works for one component (e.g. the header / menu), it will work for all components where the redundant style/string was replaced.

Release Notes

NONE

Documentation

@JanKoehnlein
Copy link
Contributor

/lgtm

@roboquat
Copy link
Contributor

LGTM label has been added.

Git tree hash: 2136cc4afb558587a52cc35fda85166a43a54c71

@JanKoehnlein
Copy link
Contributor

/approve no-issue

@roboquat
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JanKoehnlein

Associated issue requirement bypassed by: JanKoehnlein

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@gtsiolis
Copy link
Contributor

gtsiolis commented Oct 26, 2021

/werft run

👍 started the job as gitpod-build-dd-central-layout-fork.0

@roboquat roboquat merged commit ab9ad54 into gitpod-io:main Oct 26, 2021
@danieldietrich
Copy link
Contributor Author

Thank you!

@gtsiolis
Copy link
Contributor

gtsiolis commented Oct 26, 2021

Thanks for another great contribution @danieldietrich! 🧡

FYI, we've decided to embrace utility classes for the product dashboard and have been trying to avoid using arbitrary class names when possible, preferring duplication over abstraction and extracting pieces into components when needed. 🌬️

Bringing this up as semantics could change in the upcoming releases with changes like #4050, etc. 💡

Cc @jankeromnes @AlexTugarev for visibility. 👀

@danieldietrich
Copy link
Contributor Author

danieldietrich commented Oct 26, 2021

@gtsiolis thank you for the kind words and also thanks for letting me know! 🧡

This makes all sense to me. Tailwind utility classes are powerful enough, abstraction on the CSS utility level obfuscates things. In fact, I first introduced a new React component for the container layout. It encapsulated the repetition and added another feature that is currently missing: being able to style the background of a container for the full screen width. Currently the padding/margin is first applied before being able to style the background. But that's another story.

I am aware that Gitpod might completely change over time. I am looking forward to see what you are building! 🤩

@roboquat roboquat added deployed: webapp Meta team change is running in production deployed Change is completely running in production labels Oct 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved community-contribution deployed: webapp Meta team change is running in production deployed Change is completely running in production release-note-none size/M team: webapp Issue belongs to the WebApp team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants