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

Correctly handle dark icons in data-icon and data-icon-disabled #6880

Merged
merged 8 commits into from Feb 13, 2024

Conversation

zoglo
Copy link
Contributor

@zoglo zoglo commented Feb 10, 2024

Fixes #6869


Description

I extract the incoming data-attributes and properly distribute them for the two states now, this has been missing before.
Attributes get merged again but I remove the data-icons from the HtmlAttributes before doing so.


Edit

  • Just replacing the icon sources within darkAttributes works

@zoglo
Copy link
Contributor Author

zoglo commented Feb 12, 2024

/cc @leofeyer - Should we change this to 5.3 then as 5.2.9 has already been released?

@leofeyer
Copy link
Member

Yes please.

@leofeyer leofeyer added this to the 5.3 milestone Feb 12, 2024
@leofeyer
Copy link
Member

leofeyer commented Feb 12, 2024

Would it not be much shorter if we just added the following in line 210?

foreach (array('data-icon', 'data-icon-disabled') as $icon)
{
	if (isset($darkAttributes[$icon]))
	{
		$pathinfo = pathinfo($darkAttributes[$icon]);
		$darkAttributes[$icon] = $pathinfo['filename'] . '--dark.' . $pathinfo['extension'];
	}
}

$darkAttributes = new HtmlAttributes($attributes);
$darkAttributes->mergeWith(array('class' => 'color-scheme--dark', 'loading' => 'lazy'));

@zoglo zoglo marked this pull request as draft February 12, 2024 18:50
@zoglo zoglo changed the base branch from 5.2 to 5.3 February 12, 2024 18:50
@zoglo zoglo marked this pull request as ready for review February 12, 2024 19:24
@zoglo
Copy link
Contributor Author

zoglo commented Feb 12, 2024

leofeyer it's ready to review, just wanted to make sure and test it once again

@zoglo zoglo requested a review from fritzmg February 12, 2024 19:25
@zoglo
Copy link
Contributor Author

zoglo commented Feb 12, 2024

Would it not be much shorter if we just added the following in line 210?

Guess after trying out multiple approaches, I didn't see this. That works as well @leofeyer, you are right.

@leofeyer leofeyer changed the title Consider data-themes in data-icon and data-icon-disabled Correctly handle dark icons in data-icon and data-icon-disabled Feb 13, 2024
@leofeyer leofeyer merged commit 3145d8b into contao:5.3 Feb 13, 2024
17 checks passed
@leofeyer
Copy link
Member

Thank you @zoglo.

@zoglo zoglo deleted the bugfix/icons-dark-mode branch February 13, 2024 08:29
@leofeyer leofeyer linked an issue Feb 13, 2024 that may be closed by this pull request
leofeyer pushed a commit that referenced this pull request Mar 14, 2024
Description
-----------

Fixes #7018 

#6982 reverted #6880 so the logic was reintroduced within the new getHtml method

Commits
-------

f1e57ff Set dark mode data-icons (fixes #7018)
15b8634 Adjust darkAttributes logic
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wrong images/icons on darkmode on publish/unpublish
3 participants