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

New approval (proposal) card #1864 #1871

Merged
merged 12 commits into from
Jul 26, 2023

Conversation

roienatan
Copy link
Collaborator

@roienatan roienatan commented Jul 18, 2023

  • PR title equals to the ticket name
  • I added the ticket to the Development section of this PR.

What was changed?

  • Support resolutionType property for proposals and change UI accordingly.

@netlify
Copy link

netlify bot commented Jul 18, 2023

Deploy Preview for preview-common ready!

Name Link
🔨 Latest commit cce0878
🔍 Latest deploy log https://app.netlify.com/sites/preview-common/deploys/64c14b0af3f45d0008408c7b
😎 Deploy Preview https://deploy-preview-1871--preview-common.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@roienatan roienatan linked an issue Jul 18, 2023 that may be closed by this pull request
@roienatan roienatan marked this pull request as ready for review July 22, 2023 22:11
@andreymikhadyuk
Copy link
Collaborator

@roienatan Is it ready for review?

@roienatan
Copy link
Collaborator Author

@roienatan Is it ready for review?

@andreymikhadyuk now yes.

Comment on lines +8 to +10
--strong-gray: #7a819c;
--gray-60: #6b718e;
--red: #ff603e;
Copy link
Collaborator

Choose a reason for hiding this comment

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

To be honest I do not see any benefits of adding specific colors (gray, red, etc...) to the css variables.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@andreymikhadyuk Correct me if I'm wrong but the whole idea of the theme.scss was to create a new file for colors and narrow the colors we have in constants.scss. If not, what's the point of having some colors in theme.scss and some in constants.scss?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The main reason for theme to have different themes. So the names should be primary, secondary, etc... so that we can have different themes (light, dark) and we can use same name variables. But gray-30 which we use for light theme will be different for dark. As I said previously, designer should provide such specific names for themes. I do not see any reasons to stop using constants.scss and move all variables to css variables of theme.scss.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I will merge the PR, but anyway I would go further with what I described higher.

@roienatan roienatan marked this pull request as draft July 25, 2023 19:05
@roienatan roienatan marked this pull request as ready for review July 25, 2023 19:24
@andreymikhadyuk andreymikhadyuk merged commit de6f753 into dev Jul 26, 2023
6 checks passed
@andreymikhadyuk andreymikhadyuk deleted the cw-1864-proposal-resolution-type branch July 26, 2023 16:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New approval (proposal) card
2 participants