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

Fixed primefaces#16576 - Column Filter missing primary color #16588

Open
wants to merge 4 commits into
base: v18
Choose a base branch
from

Conversation

EnricoMessall
Copy link
Contributor

fix: #16576

  • CX Function add support for flatting functions for styleClass (Similar to ngStyle doing it)
  • Added new theming for p-column filter active button color
  • Added p-datatable-column-filter-button-active class again

The cx function adjustment is important so parts using [styleClass] can work with cx having a function as return value. With that the cx functions flattens the function result as the ngStyle does and directly provides styleClass with the required string. Making styleClass do that directly would require a rework of all styleClass components so that was the best option from my point of view.

 + CX Function add support for flatting functions for styleClass (Similar to ngStyle doing it)
 + Added new theming for p-column filter active button color
 + Added p-datatable-column-filter-button-active class again
Copy link

vercel bot commented Oct 18, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

2 Skipped Deployments
Name Status Preview Comments Updated (UTC)
primeng ⬜️ Ignored (Inspect) Visit Preview Nov 6, 2024 1:18pm
primeng-v18 ⬜️ Ignored (Inspect) Visit Preview Nov 6, 2024 1:18pm

@EnricoMessall EnricoMessall changed the title fix: primefaces#16576 - Column Filter missing primary color Fixed primefaces#16576 - Column Filter missing primary color Oct 18, 2024
 + Undo changes to cx function
 + Used custom styleClass transformer on styleClass inputs instead to provide safe translation always
@rosenthalj
Copy link
Contributor

rosenthalj commented Nov 5, 2024

This PR #16588 doesn't fix Issue #16576.

I download the PR branch and tested the fix. The video and annotated screenshot listed below shows that this PR doesn't fix Issue #16576: "When a table column is filtered using the p-columnFilter component there is no indication of filtering in the corresponding table column header".

stillNotFixed.mov

Angular_Table_Component

@EnricoMessall
Copy link
Contributor Author

EnricoMessall commented Nov 6, 2024

@rosenthalj You sure? In v18 the primary indicator is no longer blue, its a darker gray/black then normal stuff which can be configured with tokens. For me it looks like your icons are changed to a darker cooler what is the wanted behaviour. I've used the token "{primary.active.color}" which was blue in v17.

https://hetzner.arematics.com/assets/images/table.mov

# Conflicts:
#	src/app/components/avatar/avatar.ts
#	src/app/components/avatargroup/avatargroup.ts
#	src/app/components/blockui/blockui.ts
#	src/app/components/card/card.ts
#	src/app/components/checkbox/checkbox.ts
#	src/app/components/chip/chip.ts
#	src/app/components/confirmdialog/confirmdialog.ts
#	src/app/components/dataview/dataview.ts
#	src/app/components/divider/divider.ts
#	src/app/components/dock/dock.ts
#	src/app/components/drawer/drawer.ts
#	src/app/components/fileupload/fileupload.ts
#	src/app/components/inplace/inplace.ts
#	src/app/components/inputgroup/inputgroup.ts
#	src/app/components/inputgroupaddon/inputgroupaddon.ts
#	src/app/components/inputicon/inputicon.ts
#	src/app/components/inputmask/inputmask.ts
#	src/app/components/inputswitch/inputswitch.ts
#	src/app/components/knob/knob.ts
#	src/app/components/listbox/listbox.ts
#	src/app/components/megamenu/megamenu.ts
#	src/app/components/message/message.ts
#	src/app/components/messages/messages.ts
#	src/app/components/organizationchart/organizationchart.ts
#	src/app/components/overlaypanel/overlaypanel.ts
#	src/app/components/paginator/paginator.ts
#	src/app/components/popover/popover.ts
#	src/app/components/progressbar/progressbar.ts
#	src/app/components/progressspinner/progressspinner.ts
#	src/app/components/radiobutton/radiobutton.ts
#	src/app/components/scrollpanel/scrollpanel.ts
#	src/app/components/selectbutton/selectbutton.ts
#	src/app/components/skeleton/skeleton.ts
#	src/app/components/slider/slider.ts
#	src/app/components/splitbutton/splitbutton.ts
#	src/app/components/splitter/splitter.ts
#	src/app/components/steps/steps.ts
#	src/app/components/table/table.ts
#	src/app/components/terminal/terminal.ts
#	src/app/components/timeline/timeline.ts
#	src/app/components/toast/toast.ts
#	src/app/components/togglebutton/togglebutton.ts
#	src/app/components/toggleswitch/toggleswitch.ts
#	src/app/components/virtualscroller/virtualscroller.ts
@rosenthalj
Copy link
Contributor

@EnricoMessall

I have downloaded your PR branch with the most recent updates and there is no change, Issue #16576.

I could create a video again, but it would be exactly the same as the video that I provided in my previous comment to to this PR.

Have you tested your changes with the PrimeNG table filter-advanced demo?

You can follow the steps in the original Issue #16576:

v18_p-columnFilter__Column_filtering_indicator_is_missing_·Issue__16576·_primefaces_primeng

@EnricoMessall
Copy link
Contributor Author

@rosenthalj Look at the video i send, i couldnt upload it here because its too big for github. Its also showing this MR, you can see how the active button is applied to the filter but the color is new in PrimeNG v18 and is also black but with another level of darkness. You can also see that i change the overall primary color token and then also the selected filter color changes to the adjusted primary color.

@rosenthalj
Copy link
Contributor

@rosenthalj Look at the video i send, i couldnt upload it here because its too big for github. Its also showing this MR, you can see how the active button is applied to the filter but the color is new in PrimeNG v18 and is also black but with another level of darkness. You can also see that i change the overall primary color token and then also the selected filter color changes to the adjusted primary color.

@EnricoMessall can you try saving your video at a lower resolution or provide screenshots? I can not verify the results of your PR. Somehow/Somewhere something is obviously wrong or inconsistent?

@rosenthalj
Copy link
Contributor

@rosenthalj Look at the video i send, i couldnt upload it here because its too big for github. Its also showing this MR, you can see how the active button is applied to the filter but the color is new in PrimeNG v18 and is also black but with another level of darkness. You can also see that i change the overall primary color token and then also the selected filter color changes to the adjusted primary color.

@EnricoMessall can you try saving your video at a lower resolution or provide screenshots? I can not verify the results of your PR. Somehow/Somewhere something is obviously wrong or inconsistent?

@EnricoMessall another idea is to split your video into two videos and upload them separately

@EnricoMessall
Copy link
Contributor Author

@rosenthalj Look into the primeng discord. I've messaged you there with the video.

@rosenthalj
Copy link
Contributor

@EnricoMessall

Your video on PrimeNG Discord is nice and I now understand what you are trying to convey.

I confirmed that after your changes, the p-datatable-column-filter-button-active class is now being added to the filter button. This is similar to v17. PrimeNG v17 uses the p-column-filter-menu-button-active class for the filter button. It appears that PrimeNG v18 is changing a lot of CSS class names.

Unfortunately, even with your changes, the PrimeNG Table Filter Advance Demo is still not working properly. As a result, any software upgraded from 17.x to 18.x will still not have any visual indication when the table has been filtered unless the developer realizes its now their responsibility to fix table filter visual indicators.

Note on your video: You were eventually able to change the filter button color by using the browser's "inspection" functionality. In v17 both the filter button color and the filter button background colors are changed to indicate a column has been filtered.

@rosenthalj
Copy link
Contributor

I believe the current proposed changes for this PR enables hooks that enables developers to implement their own custom CSS as a workaround to fix Issue #16576. The PR is not ideal but is better than the current implemented code.

@mertsincan
Copy link
Member

Hi @EnricoMessall,

Thanks a lot for your contribution, but this PR has conflicts. Could you please update your PR? Then, I'll review it asap. Thanks again!

@mertsincan mertsincan added the Resolution: Needs Revision The pull request can't be merged. Conflicts need to be corrected or documentation and code updated. label Nov 18, 2024
@mertsincan mertsincan linked an issue Nov 18, 2024 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Resolution: Needs Revision The pull request can't be merged. Conflicts need to be corrected or documentation and code updated.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

v18 p-columnFilter: Column filtering indicator is missing
3 participants