Skip to content

Conversation

@danyill
Copy link
Contributor

@danyill danyill commented Dec 23, 2024

Closes #5

This is a "cheap and cheerful" fix.

We:

  • Don't allow breaking of the first line -- it becomes confusing to read
  • Prevent variable height rows - it will not work with the buttons - perhaps it should be qualified more
  • Introduce a new property for height on action-list
  • Improve spacing on lists to make action icons more evenly spaced
  • Add one small test (updated baseline for Chrome only - assuming these will need to be regenerated anyway)
  • The way I've added the styling is as per https://lit.dev/docs/components/styles/#style-element

I don't think this will terribly break compatibility with other uses but please consider this.

I will update the PR aligned with this in oscd-publisher OpenEnergyTools/oscd-publisher#7 and provide some screenshots for assessment so that with an npm link it can be examined more closely

@danyill danyill force-pushed the issue-5-icon-position-spacing branch from 6cef6cb to a0f506a Compare December 23, 2024 19:13
@danyill
Copy link
Contributor Author

danyill commented Dec 23, 2024

I would be grateful for a review and/or discussion on this

@danyill
Copy link
Contributor Author

danyill commented Dec 24, 2024

More updates on this PR soon! Don't look yet 😉 @ca-d helped me improve it muchly and tomorrow I will update and add some commentary.

@danyill danyill force-pushed the issue-5-icon-position-spacing branch from 7f0dcda to 31260d2 Compare December 24, 2024 10:38
@danyill
Copy link
Contributor Author

danyill commented Dec 24, 2024

We've added some:

  • theme styling
  • headline/supporting text now slotted correctly

I'd be grateful for a review at an appropriate time.

@JakobVogelsang
Copy link
Contributor

Hi Dan, I think it look promising. I just did scope thie element and added visual regresion test to run on a PR. I would ask you taking over the changes into the new main and let the github action generate the new screenshots. Sorry for the unconvinience. I want though to have that feature in.

@JakobVogelsang
Copy link
Contributor

Does this PR solve you problems with the different list element hights?

@danyill
Copy link
Contributor Author

danyill commented Jan 5, 2025

I would ask you taking over the changes into the new main and let the github action generate the new screenshots. Sorry for the unconvinience.

I have merged it (it wasn't clean so I had to do somewhat by hand, hopefully without error).

Nice to see your new scoped components and the approach there.

Does this PR solve you problems with the different list element hights?

Yes, I think this will keep the heights correct.

@danyill danyill force-pushed the issue-5-icon-position-spacing branch from 8625a7e to da86815 Compare January 21, 2025 09:17
@danyill danyill force-pushed the issue-5-icon-position-spacing branch from da86815 to 4460c9a Compare January 21, 2025 09:24
@danyill
Copy link
Contributor Author

danyill commented Jan 21, 2025

Just to note that I've removed open-scd theme styling as requested and included the artifacts from the CI pipeline. Thanks for the (in-person) review.

@JakobVogelsang JakobVogelsang merged commit b47e720 into OpenEnergyTools:main Jan 28, 2025
2 checks passed
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.

Ensure icons are correctly positioned when space constrained

2 participants