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

Move css to component scope #2168

Closed
wants to merge 24 commits into from

Conversation

farhanlatheef
Copy link
Contributor

@farhanlatheef farhanlatheef commented Apr 16, 2024

Description

  • Changed: Moved button css to component level.

Checklist

  • I have made corresponding changes to the documentation.
    - [ ] I have updated the types definition of modified exports.
  • I have verified the functionality in some of the neeto web-apps.
    - [ ] I have added tests that prove my fix is effective or that my feature works.
    - [ ] I have added proper data-cy and data-testid attributes.
  • I have added the necessary label (patch/minor/major - If package publish
    is required).

Reviewers

@neetodeploy neetodeploy bot temporarily deployed to neeto-ui-kpyu-pr-2168 April 16, 2024 13:43 Inactive
@farhanlatheef farhanlatheef added the minor Releases non-breaking noteworthy changes with backward compatible. label Apr 16, 2024
@neetodeploy neetodeploy bot temporarily deployed to neeto-ui-kpyu-pr-2168 April 16, 2024 13:51 Inactive
@neetodeploy neetodeploy bot temporarily deployed to neeto-ui-kpyu-pr-2168 April 16, 2024 13:55 Inactive
@neetodeploy neetodeploy bot had a problem deploying to neeto-ui-kpyu-pr-2168 April 16, 2024 14:01 Failure
@neetodeploy neetodeploy bot temporarily deployed to neeto-ui-kpyu-pr-2168 April 16, 2024 14:04 Inactive
@neetodeploy neetodeploy bot temporarily deployed to neeto-ui-kpyu-pr-2168 April 16, 2024 14:05 Inactive
@neetodeploy neetodeploy bot had a problem deploying to neeto-ui-kpyu-pr-2168 April 16, 2024 14:06 Failure
@neetodeploy neetodeploy bot temporarily deployed to neeto-ui-kpyu-pr-2168 April 16, 2024 14:18 Inactive
@neetodeploy neetodeploy bot temporarily deployed to neeto-ui-kpyu-pr-2168 April 17, 2024 07:09 Inactive
@neetodeploy neetodeploy bot temporarily deployed to neeto-ui-kpyu-pr-2168 April 17, 2024 07:10 Inactive
@neetodeploy neetodeploy bot temporarily deployed to neeto-ui-kpyu-pr-2168 April 17, 2024 07:12 Inactive
@neetodeploy neetodeploy bot temporarily deployed to neeto-ui-kpyu-pr-2168 April 17, 2024 07:17 Inactive
@neetodeploy neetodeploy bot temporarily deployed to neeto-ui-kpyu-pr-2168 April 17, 2024 07:35 Inactive
@neetodeploy neetodeploy bot temporarily deployed to neeto-ui-kpyu-pr-2168 April 17, 2024 07:42 Inactive
@farhanlatheef farhanlatheef added the major Releases breaking changes. label Apr 18, 2024
@farhanlatheef
Copy link
Contributor Author

Copy link
Contributor

@josephmathew900 josephmathew900 left a comment

Choose a reason for hiding this comment

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

@farhanlatheef _a LGTM.

  1. As we discussed in the call, once this PR https://github.com/bigbinary/neeto-commons-frontend/pull/1139 is merged, either we should move the CSS of all the components to its respective files or we need to find a workaround for other components CSS to work.
  2. Add an empty stylesheet so that the @import "@bigbinary/neetoui" won't break.
  3. I checked the bundle and everything LGTM. I don't see any code duplication. But check the HTML head also to ensure that multiple imports don't cause multiple styles to get inserted.
  4. Once this is done, please have a call with @praveen-murali-ind and explain the logic so that he can do further optimization.

@neetodeploy neetodeploy bot temporarily deployed to neeto-ui-kpyu-pr-2168 April 19, 2024 10:31 Inactive
@neetodeploy neetodeploy bot temporarily deployed to neeto-ui-kpyu-pr-2168 April 19, 2024 14:25 Inactive
@farhanlatheef farhanlatheef changed the title Button: move css to component Move css to component scope Apr 19, 2024
Copy link
Contributor

neetogit-bot bot commented Apr 19, 2024

@farhanlatheef CI is red.

@neetodeploy neetodeploy bot temporarily deployed to neeto-ui-kpyu-pr-2168 April 19, 2024 15:18 Inactive
@neetodeploy neetodeploy bot temporarily deployed to neeto-ui-kpyu-pr-2168 April 19, 2024 15:32 Inactive
@neetodeploy neetodeploy bot temporarily deployed to neeto-ui-kpyu-pr-2168 April 22, 2024 05:16 Inactive
@neetodeploy neetodeploy bot temporarily deployed to neeto-ui-kpyu-pr-2168 April 22, 2024 05:18 Inactive
@neetodeploy neetodeploy bot temporarily deployed to neeto-ui-kpyu-pr-2168 April 22, 2024 05:19 Inactive
@farhanlatheef
Copy link
Contributor Author

@praveen-murali-ind _a

@praveen-murali-ind
Copy link
Contributor

Found some edge cases of this approach.

  1. neetoUI provides helper classes for managing the text color, background color, border color, flex, text alignment etc. Since the styles are clubbed with the component in this approach, if a page is not using any neetoUI components, these utility classes won’t work. We are using these classes everywhere in neeto products.

  2. Some of the neetoUI component classes are being used in host applications to reuse the neetoUI styles. In the new approach, reusing the component classes won’t work if the neetoUI component is not being used on that page.

I had a discussion with @josephmathew900 and @AbhayVAshokan regarding these issues. We will check the feasibility of purging the unused CSS instead of the component-scoped CSS approach. Closing this PR and related PRs in host applications. Will do a separate POC for purging CSS for more optimisations.

cc @farhanlatheef

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dont-merge minor Releases non-breaking noteworthy changes with backward compatible.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants