[WALL / WIP] Aizad/Added Quill Components inside of Core and Wallets #12110
Conversation
aizad-deriv
requested review from
adrienne-deriv,
farhan-nurzi-deriv and
thisyahlen-deriv
as code owners
aizad-deriv
requested review from
balakrishna-deriv,
heorhi-deriv and
yashim-deriv
as code owners
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
A production App ID was automatically generated for this PR. (log)
Click here to copy & paste above information. |
|
🚨 Lighthouse report for the changes in this PR:
Lighthouse ran with https://deriv-app-git-fork-aizad-deriv-aizad-quill-implementation.binary.sx/ |
|
Kudos, SonarCloud Quality Gate passed!
|
|
❌ Smoke test run (2) failed. See logs for details: Visit Action |
|
❌ Smoke test run (1) failed. See logs for details: Visit Action |
|
Will install CVA to handle the different types of variations and colors for the component in the near future. |
| "stylelint-no-unsupported-browser-features": "^4.0.0", | ||
| "stylelint-selector-bem-pattern": "^2.1.0", | ||
| "stylelint-webpack-plugin": "^2.1.1", | ||
| "tailwindcss": "^3.3.6", |
There was a problem hiding this comment.
@aizad-deriv can't we use quill components without using tailwind?
There was a problem hiding this comment.
yes you can, but TBH adding them won't harm anybody.
if you guys want to use the components only, it's cool too.
There was a problem hiding this comment.
Will try this out and see if its needed. Will update here base on the findings @mohsen-deriv @mahdiyeh-deriv 🙏🏼
| @import '@deriv/quill-design/quill-tailwind/fonts.css'; | ||
|
|
||
| @import './styles/devices.scss'; | ||
| svg { |
There was a problem hiding this comment.
let'e don't merge quill icons and components in one PR.
There was a problem hiding this comment.
this is not about quill icons, there has been a conflict between deriv-app and quill css reset, this fixes the conflict.
There was a problem hiding this comment.
There was some issues when applying the base styles, @tailwind base and it is affecting svgs. We resolved it by adding this css rule to resolve the issue however this is a temporary solution. Ideally there is no need to add css.
| <div className='wallets-icon-button__icon'>{icon}</div> | ||
| <button | ||
| className={qtMerge( | ||
| `inline-flex align-middle justify-center hover:cursor-pointer ${ |
There was a problem hiding this comment.
hmm, that looks very much like dreaded inline css :D
I think that either should be removed (as per previous @mahdiyeh-deriv comment), or if we actually want to introduce tailwind, that should be discussed/approved in bigger group.
cc @markwylde-deriv
There was a problem hiding this comment.
I agree there is some cons to this like you mentioned the long classnames however I suggest we scoped it to the components that we are using in Wallets and see what is the reaction of the other developers.
5 participants
🏗️ In Progress / Research
Shoutout to @mohsen-deriv for the help and support 🙇🏼
Changes:
@deriv/quill-designand Tailwind inside ofcoreand enabling it insidewalletstailwind.config.jsto bothcoreandwalletsindex.scssIconButtoncomponent to usequill-componentsScreenshots: