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

Add option to make active list filters more visible #178

Merged

Conversation

merwok
Copy link
Contributor

@merwok merwok commented Sep 28, 2022

Fixes #174

@merwok
Copy link
Contributor Author

merwok commented Sep 28, 2022

Active filter headings are now bold, and selected values use module colours (default style) or bold too (dropdown style):

dai-filters-default dai-filters-dropdown

@codecov
Copy link

codecov bot commented Sep 28, 2022

Codecov Report

Base: 98.13% // Head: 98.16% // Increases project coverage by +0.02% 🎉

Coverage data is based on head (d0b3d7e) compared to base (04df53e).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #178      +/-   ##
==========================================
+ Coverage   98.13%   98.16%   +0.02%     
==========================================
  Files          34       35       +1     
  Lines         376      381       +5     
==========================================
+ Hits          369      374       +5     
  Misses          7        7              
Flag Coverage Δ
unittests 98.16% <100.00%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...ace/migrations/0026_theme_list_filter_highlight.py 100.00% <100.00%> (ø)
admin_interface/models.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

fabiocaccamo
fabiocaccamo previously approved these changes Sep 29, 2022
@fabiocaccamo
Copy link
Owner

fabiocaccamo commented Sep 29, 2022

@merwok thank your very much also for this PR, very cool and useful!

Do you still need to work on this PR or I can merge it?

@merwok
Copy link
Contributor Author

merwok commented Sep 29, 2022

The PR contains item A from the discussion, I will add item C too!

I also wanted feedback about the styles from you and from my client who’s funding this work 🙂

@fabiocaccamo
Copy link
Owner

I also wanted feedback about the styles from you and from my client who’s funding this work

My personal feedback on style:

  • I think the padding between the selected option and the list filter container should be the same on left and right.

  • If there is horizontal space between the selected option and the container, consider rounding border radius of the selected option using the css variable.

  • On the left of the selected item, a very soft grey marker is still visible, when list filter highlight is enabled it should be hidden or of the same color of the selected option.


I'm glad for you that your client is funding this work!
As you can see this project is widely used, but unfortunately it has few contributors and no sponsors at all.

@fabiocaccamo
Copy link
Owner

The PR contains item A from the discussion, I will add item C too!

I think now that item A and C are two different features and probably it's better to have the possibility to enable them separately, do you agree?

@merwok merwok force-pushed the feature/active-filters branch from 48fd5fd to c50b2b5 Compare September 30, 2022 13:25
@merwok
Copy link
Contributor Author

merwok commented Sep 30, 2022

it's better to have the possibility to enable them separately

I was already planning to have two different settings, and we can also have two PRs.

@merwok
Copy link
Contributor Author

merwok commented Sep 30, 2022

Thanks for the feedback! I will try to apply it after a weekend, unless you want to do it quickly and add commits to this branch.

@fabiocaccamo
Copy link
Owner

No hurry, have a nice weekend!

@fabiocaccamo
Copy link
Owner

@merwok maybe we could start to merge this PR?

@merwok
Copy link
Contributor Author

merwok commented Oct 5, 2022

I think the CSS tweaks need to be addressed!

fabiocaccamo
fabiocaccamo previously approved these changes Oct 5, 2022
@fabiocaccamo
Copy link
Owner

@merwok I agree, I wait for the CSS changes.

@merwok
Copy link
Contributor Author

merwok commented Oct 6, 2022

I have applied changes for the three points:

list-filters-revised

For this one:

If there is horizontal space between the selected option and the container,

I’m not sure what you mean by that, so I just added a border radius unconditionally (if the highlight option is enabled).

@merwok merwok marked this pull request as ready for review October 6, 2022 02:43
@fabiocaccamo
Copy link
Owner

I looks perfect to me!
I noticed only now that I reviewed the PR while it was still in draft, sorry.


I’m not sure what you mean by that, so I just added a border radius unconditionally (if the highlight option is enabled).

I meant this padding:

194202262-6daf02ad-5d68-4825-9c13-3aebe71fce3d

@fabiocaccamo fabiocaccamo merged commit f25d340 into fabiocaccamo:master Oct 6, 2022
@merwok merwok deleted the feature/active-filters branch October 6, 2022 14:01
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.

Make currently active list filters more visible
2 participants