Skip to content
This repository has been archived by the owner on Jan 16, 2022. It is now read-only.

feat: Added Theme and migrate to emotion@10.x 馃殌 #286

Merged
merged 8 commits into from Nov 23, 2019

Conversation

priscilawebdev
Copy link
Contributor

@priscilawebdev priscilawebdev commented Nov 18, 2019

Type: Feat + Refactor + Chore (sorry)

Description:

I have added the emotion theme together with the Material Ui Theme. Since I had some problems due to the old version of emotion, I had to update the all the emotion dependencies and fix all the breaking changes.

Of course, there's a lot to improve, but it's a start 馃檭

PS: As soon as they release the fix for this issue emotion-js/emotion#1305, I will remove the <{theme?: Theme}> and add a file a file with

import styled, { CreateStyled } from '@emotion/styled';

import { Theme } from './theme';

export default styled as CreateStyled<Theme>;

@codecov
Copy link

codecov bot commented Nov 18, 2019

Codecov Report

Merging #286 into master will decrease coverage by 1.15%.
The diff coverage is 91.83%.

@@            Coverage Diff             @@
##           master     #286      +/-   ##
==========================================
- Coverage   90.31%   89.15%   -1.16%     
==========================================
  Files         140      141       +1     
  Lines         929      959      +30     
  Branches      143      177      +34     
==========================================
+ Hits          839      855      +16     
- Misses         78       93      +15     
+ Partials       12       11       -1
Impacted Files Coverage 螖
src/components/Help/styles.ts 100% <酶> (酶) 猬嗭笍
src/components/RegistryInfoContent/styles.ts 100% <酶> (酶) 猬嗭笍
src/components/Tag/styles.ts 100% <酶> (酶) 猬嗭笍
src/components/Login/Login.tsx 91.48% <酶> (酶) 猬嗭笍
src/components/Dependencies/styles.ts 100% <酶> (酶) 猬嗭笍
src/components/Install/InstallListItem.tsx 88.88% <酶> (酶) 猬嗭笍
src/components/Login/styles.ts 100% <酶> (酶) 猬嗭笍
src/components/Install/Install.tsx 85.71% <酶> (酶) 猬嗭笍
...onents/RegistryInfoContent/RegistryInfoContent.tsx 90% <酶> (酶) 猬嗭笍
src/components/DetailSidebar/styles.ts 100% <酶> (酶) 猬嗭笍
... and 37 more

@anikethsaha
Copy link
Member

Just for future tracking, referencing the migration issue https://github.com/verdaccio/ui/issues/221

@anikethsaha
Copy link
Member

BTW, The migration to emotion 10 looking great 馃

Copy link
Contributor

@tmkn tmkn left a comment

Choose a reason for hiding this comment

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

If we want to enable strict mode then we shouldn't introduce old errors.


import CopyToClipBoard from './CopyToClipBoard';
import { CopyIcon } from './styles';

jest.mock('../../utils/cli-utils');

describe('<CopyToClipBoard /> component', () => {
let wrapper: ReactWrapper;
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason why you removed the type here? This now errors again in strict mode, it was already covered :(

Copy link
Member

Choose a reason for hiding this comment

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

It might be a mistake, I restored them.

Copy link
Member

Choose a reason for hiding this comment

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

Restored, thanks @tmkn for pointing it out

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry @tmkn I was getting errors due to these types, I removed it for testing purposes and forgot to undo my changes ... good catch!

Copy link
Contributor

Choose a reason for hiding this comment

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

@priscilawebdev no worries, I was sure it wasn't intentional, that's what code reviews are for :)

src/App/App.test.tsx Outdated Show resolved Hide resolved
Copy link
Member

@juanpicado juanpicado left a comment

Choose a reason for hiding this comment

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

I am reviewing this.

Co-Authored-By: Thomas Klein <tmkn@users.noreply.github.com>
@verdacciobot
Copy link

Thanks for your PR, the @verdaccio/ui package will be accessible from here for testing purposes:

npm install @verdaccio/ui-theme@v0.3.7-e7717d8-pr286.0 --registry https://registry.verdaccio.org

Copy link
Member

@juanpicado juanpicado left a comment

Choose a reason for hiding this comment

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

Great @priscilawebdev it is perfect, the UI looks the same, great migration.

@juanpicado juanpicado changed the title Feature: Added Theme 馃殌 feat: Added Theme and migrate to emotion@10.x 馃殌 Nov 23, 2019
@juanpicado
Copy link
Member

Thanks @anikethsaha and @tmkn for the early reviews 馃殌

@juanpicado juanpicado merged commit 111f0c5 into master Nov 23, 2019
@delete-merged-branch delete-merged-branch bot deleted the feature/add_mui_theme branch November 23, 2019 12:41
@priscilawebdev
Copy link
Contributor Author

thank you team 馃馃挭

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
5 participants