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

Lots of warning in the browser console related to swatches #4217

Closed
addison74 opened this issue Sep 21, 2024 · 10 comments · Fixed by #4218
Closed

Lots of warning in the browser console related to swatches #4217

addison74 opened this issue Sep 21, 2024 · 10 comments · Fixed by #4218
Assignees

Comments

@addison74
Copy link
Contributor

Go to Backend > Attributes > Manage Attributes and edit the Color attribute. Go to the Manage Label / Options tab on the left side. With the sample pack there are 19 options, from Black to Yellow.

Open the browser console. You will see 19 warning related to the value "false" used in the swatches. The screenshot is bellow

screenshot4

As a personal opinion, since I did not follow the PR that made this change, it would have been preferable for that ugly X to be on a new column, next to the swatch or on the last column. This is how the change would have looked for the last column. Even a

screenshot5

Even without text and it would have looked better compared to the other elements on the page

screenshot6

@sreichel
Copy link
Contributor

@empiricompany maybe you can review this?

@empiricompany
Copy link
Contributor

I look this fast cause I am on mobile and seems by your screenshots that you didn't load all the css and javascript correctly
But I could be wrong, I gotta investigate

@empiricompany
Copy link
Contributor

I checked, and on Firefox, it doesn't give any warnings, while on Chrome it does. They can be safely ignored considering the benefit they bring, as the warnings do not compromise functionality.

This is how it should look:

demo

We spent a lot of time finding a solution for both Windows and Mac with some compromises to ensure maximum backward compatibility, and to be completely honest, I have no desire to go back and work on the code again.

This was the PR where also you @addison74 have followed: #3686

@addison74
Copy link
Contributor Author

I appreciate you checking this report. Indeed I followed it until a certain moment when I gave up for some reasons.

The problem is visible in Chromium, so in Chrome, Edge, Brave. Maybe we can all find a solution because this is the only section where I found warnings in the console.

Regarding that X, the legacy theme from OpenMage must be taken into account. I say it as a personal opinion and if someone is interested in doing it, it would be a useful change. For this purpose I would reopen the report to be seen by others. Thank you for your understanding.

@sreichel
Copy link
Contributor

sreichel commented Sep 21, 2024

@addison74 make sure to refresh browser cache. Backend looks fine for me (Firefox).

(If it does not work with Chrome, it should be checked)

I'd care about the console warnings - if there are any. (not tested yet)

@addison74
Copy link
Contributor Author

addison74 commented Sep 21, 2024

@sreichel - I always use browsers with cache disabled. Before reporting this issue, related to Chromium flavors, I cleared the cache to be sure.

@addison74 addison74 reopened this Sep 22, 2024
@sreichel
Copy link
Contributor

sreichel commented Sep 22, 2024

Confirmed.

@empiricompany the broken "X" images are in legacy theme, not in the new openmage.

Console warnings are only in Chrome, not in Firefox.

@empiricompany
Copy link
Contributor

empiricompany commented Sep 22, 2024

Do we still need to support the legacy theme?
IMHO legacy theme it was kept only for any exceptional backward compatibility needs.
It is impossible for every PR to control it in both themes, it makes no sense.

Use the official OpenMage backend theme by default and there are no problems.

@OpenMage OpenMage locked and limited conversation to collaborators Sep 22, 2024
@sreichel sreichel assigned sreichel and unassigned empiricompany Sep 22, 2024
@sreichel
Copy link
Contributor

sreichel commented Sep 22, 2024

As long we have that theme, it should be supported.

Working on it.

@sreichel sreichel reopened this Sep 22, 2024
@colinmollenhour
Copy link
Member

As of now I think it is legitimate to want to fix bugs in the legacy theme as long as it exists in the main repo. However, there is a good case to be made for dropping support of it. To that end, I have opened a discussion here: #4230
Let's leave this issue open for now, there is no harm in someone volunteering to fix it. If we decide to drop support, then in the future these types of issues would either not exist or be reported on the secondary repo only.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants