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

Always use svg series icon #5214

Merged
merged 7 commits into from
Jan 9, 2024
Merged

Always use svg series icon #5214

merged 7 commits into from
Jan 9, 2024

Conversation

jorg-vr
Copy link
Contributor

@jorg-vr jorg-vr commented Dec 4, 2023

This pull request fixes #5213

It were actually to separate bugs:

  1. the fallback for non signed in users was the simplified icon
  2. empty series were not postloaded

The fix removes the simplified icon, always using the svg version.
The amount of html remains limited for non loaded series because the svg code is moved to a web component.

Closes #5213

@jorg-vr jorg-vr added the bug Something isn't working label Dec 4, 2023
@jorg-vr jorg-vr self-assigned this Dec 4, 2023
@jorg-vr jorg-vr marked this pull request as ready for review December 4, 2023 13:34
@jorg-vr jorg-vr requested a review from a team as a code owner December 4, 2023 13:34
@jorg-vr jorg-vr requested review from bmesuere and chvp and removed request for a team December 4, 2023 13:34
@bmesuere bmesuere added the deploy naos Request a deployment on naos label Dec 4, 2023
@github-actions github-actions bot removed the deploy naos Request a deployment on naos label Dec 4, 2023
@jorg-vr jorg-vr changed the title Only use simplified icon when series is not yet loaded Always use svg series icon Dec 5, 2023
@chvp chvp added the deploy naos Request a deployment on naos label Dec 5, 2023
@github-actions github-actions bot removed the deploy naos Request a deployment on naos label Dec 5, 2023
Copy link
Member

@chvp chvp left a comment

Choose a reason for hiding this comment

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

The tooltip location seems off: screenshot_2023-12-05-132544

@jorg-vr jorg-vr requested a review from chvp December 5, 2023 13:14
@chvp chvp added the deploy naos Request a deployment on naos label Dec 5, 2023
@github-actions github-actions bot removed the deploy naos Request a deployment on naos label Dec 5, 2023
@bmesuere bmesuere added the deploy mestra Request a deployment on mestra label Dec 19, 2023
@github-actions github-actions bot removed the deploy mestra Request a deployment on mestra label Dec 19, 2023
@bmesuere bmesuere added the deploy naos Request a deployment on naos label Dec 19, 2023
@github-actions github-actions bot removed the deploy naos Request a deployment on naos label Dec 19, 2023
app/assets/javascripts/components/series_icon.ts Outdated Show resolved Hide resolved
Comment on lines +10 to +13
* @cssprop --icon-color - the color of the icon
* @cssprop --icon-background-color - the background color of the icon
* @cssprop --deadline-icon-color - the color of the deadline icon
* @cssprop --deadline-icon-background-color - the background color of the deadline icon
Copy link
Member

Choose a reason for hiding this comment

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

Are these specified in a general css file? Why not add them here to make this component self contained?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right now it is a shadowless lit component, which forces all style definitions to be in the global scope.
Changing it to use the shadow dom would not allow any use of globally defined classes. In this this would mean we had to redefine the material design icons used within this class. I do not think that is desired.

Sadly there is no way to 'partially' use the shadow dom. Which is why we also aren't using the shadow dom in most other components for now.

@jorg-vr jorg-vr marked this pull request as draft January 3, 2024 08:40
@jorg-vr jorg-vr marked this pull request as ready for review January 3, 2024 11:41
@jorg-vr jorg-vr requested review from bmesuere and chvp January 3, 2024 11:41
@jorg-vr jorg-vr merged commit 0b6619a into main Jan 9, 2024
11 of 13 checks passed
@jorg-vr jorg-vr deleted the fix/series-icon branch January 9, 2024 14:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Series icon layout is missing in some cases
3 participants