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

Migrate 'atom-ui' into core #18484

Merged
merged 8 commits into from Nov 29, 2018

Conversation

Projects
None yet
1 participant
@simurai
Copy link
Member

simurai commented Nov 23, 2018

⚠️ Wait with merging until the next release is out, so we can test this a bit longer.

Description of the Change

This PR moves atom-ui back into static/. In addition, core styles and icons got their own folders.

  • Move atom-ui styles
  • Remove atom-ui dependency
  • Move core styles to static/core-ui/
  • Move icons to static/icons/

After this PR is merged.

Benefits

It's part of the package migration initiative.

Originally atom-ui got moved to its own repo in hopes of increased community contribution. Unfortunately that didn't happen. Also, once the styleguide package gets migrated, it will be easier to make updates since they are both part of the same repo.

Possible Drawbacks

There is a chance for regressions.

Verification Process

  1. Open Atom in dev mode: atom . -d
  2. Verify that the UI still looks the same.
  3. Verify that there are no errors in the console.
  4. Verify that the icons get rendered.

Applicable Issues

N/A

Release Notes

N/A

Import fallback variables
To see if it fixes the missing @use-custom-controls in less-cache
@simurai

This comment has been minimized.

Copy link
Member

simurai commented Nov 26, 2018

The build keeps failing with:

Generating pre-built less cache in /Users/simurai/github/atom/out/app/less-compile-cache

/Users/simurai/github/atom/node_modules/less-cache/lib/less-cache.js:296
            throw error;
            ^
Error: variable @use-custom-controls is undefined

I think it's because all files found in static/ get compiled "individually" and not based on the import paths. Like the @use-custom-controls gets imported first and would be available, but less-cache doesn't know about it.

I guess we have two options:

  1. Move the atom-ui styles somewhere outside of static/. Then less-cache doesn't check them automatically.
  2. Make sure that every .less file imports all the variables, even though they get already imported before.

I might try option 2, since caching more styles sounds like it adds extra benefits to performance.

@asheren asheren referenced this pull request Nov 27, 2018

Closed

Iteration Plan: November 26 - December 7 #18504

0 of 6 tasks complete
@simurai

This comment has been minimized.

Copy link
Member

simurai commented Nov 29, 2018

Ok, this seems good. I'll merge now, so we still have till January to catch any regressions.

@simurai simurai merged commit 08063d6 into master Nov 29, 2018

3 checks passed

Atom Pull Requests #20181128.1 succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@simurai simurai deleted the sm/atom-ui branch Nov 29, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment