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

Edit opacity from 0.8 to 0.5 and remove forBackdropFilter #5291

Merged
merged 7 commits into from
May 16, 2024

Conversation

jss475
Copy link
Contributor

@jss475 jss475 commented May 4, 2024

Update for #4836

  • edit primary and secondary transparency opacities from 0.8 to 0.5
  • remove forBackdropFilter from themes
  • update components referencing transparency/primary and transparency/secondary to have the following backdrop-filter: blur(12px) saturate(200%) contrast(50%) brightness(130%)

@FelixMalfait
Copy link
Member

Thanks a lot @jss475!
Your PR looks great. But I think we need to fix dark mode, it becomes even more obvious now with the changes you made

@Bonapara can you confirm adjustments we should make to dark mode?

After:
Screenshot 2024-05-04 at 15 47 45
Screenshot 2024-05-04 at 15 48 59
Screenshot 2024-05-04 at 15 51 04

Before:
Screenshot 2024-05-04 at 15 48 17
Screenshot 2024-05-04 at 15 49 25

@Bonapara
Copy link
Member

Bonapara commented May 6, 2024

Dark mode should look like this:

CleanShot 2024-05-06 at 10 45 55

To do:

  • Remove the hardcoded background on search inputs that makes it look darker (Set background: transparent).
  • Remove the border-top on the input when It's at the top of the menu:

CleanShot 2024-05-06 at 10 48 44

  • Make the input 36px instead of 38px+ (with the border)

Current behavior ↓

CleanShot 2024-05-06 at 10 54 12

@FelixMalfait
Copy link
Member

Would you mind handling those changes in the PR @jss475 🙏? Thanks a lot

@jss475
Copy link
Contributor Author

jss475 commented May 6, 2024

Yup you got it @FelixMalfait!

… remove top border, input space height: 36px
@jss475
Copy link
Contributor Author

jss475 commented May 6, 2024

@Bonapara I hardcoded the background for the ObjectFilterDropdownFilterSelect component to have background: transparent, made border-top: none, and edited the height to be 36px.

Should I have conditionally made the background transparent only in dark mode and have a different conditional to make border-top none when its at the top position?

Also, is the intended behavior to have a small padding on the bottom if there is no text that matches the filter text?

Screenshot 2024-05-06 at 4 48 34 PM

@Bonapara
Copy link
Member

Bonapara commented May 7, 2024

Hi @jss475,

I think background:transparent should be in both light and dark mode.
Also, can you check if we need to add a border-bottom to the menu header if you remove the border-top on the search?

CleanShot 2024-05-07 at 10 09 22

Regarding the padding, it should be 8px left and right, and the height should be fixed to 36px:

You can inspect this component:

https://www.figma.com/file/xt8O9mFeLl46C5InWwoMrN/Twenty?type=design&node-id=2979-174004&mode=design&t=XnegKqWtGbRaLmeE-11

Thanks!

@jss475
Copy link
Contributor Author

jss475 commented May 7, 2024

Hey @Bonapara!

You got it! I had to add a bottom-border for the menu header. I've updated it with the latest commit.

Thanks!

Copy link
Member

@FelixMalfait FelixMalfait left a comment

Choose a reason for hiding this comment

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

Thanks a lot!

@FelixMalfait FelixMalfait merged commit 9bc9513 into twentyhq:main May 16, 2024
13 checks passed
arnavsaxena17 pushed a commit to arnavsaxena17/twenty that referenced this pull request Oct 6, 2024
)

Update for twentyhq#4836 

- edit primary and secondary transparency opacities from 0.8 to 0.5
- remove forBackdropFilter from themes
- update components referencing transparency/primary and
transparency/secondary to have the following backdrop-filter: blur(12px)
saturate(200%) contrast(50%) brightness(130%)

---------

Co-authored-by: Félix Malfait <felix.malfait@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants