Skip to content

Refactors widget attribute and refresh handling #9101

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

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

saqimtiaz
Copy link
Member

@saqimtiaz saqimtiaz commented Jun 17, 2025

Work in progress:

This is an exploratory PR to see if a refactor of the base widget class that changes how we calculate and refresh attributes towards a more declarative style can be made while staying backwards compatible.

  • This PR adds support for data- and style. attributes for the image widget, correcting an omission in Add data attribute support to button and other widgets #7769
  • The refresh handling is also improved to allow dynamic updating of the width, height, usemap, alt and tooltip attributes without needing to re-render the widget.
  • There is an underlying change to the widget base class method assignAttributes which allows it to accept an additional hashmap (additionalAttributesMap) of mappings from widget attributes to DOM attributes. This can be used to simplify both the assignment of DOM attributes as well as their refresh.
    • this technique can also be applied to other widgets and should aid maintainability of code while possibly making it more concise.

Copy link

Confirmed: saqimtiaz has already signed the Contributor License Agreement (see contributing.md)

Copy link

github-actions bot commented Jun 17, 2025

📊 Build Size Comparison: empty.html

Branch Size
Base (master) 2528.0 KB
PR 2529.4 KB

Diff: ⬆️ Increase: +1.4 KB

Copy link

netlify bot commented Jun 17, 2025

Deploy Preview for tiddlywiki-previews ready!

Name Link
🔨 Latest commit 4382c69
🔍 Latest deploy log https://app.netlify.com/projects/tiddlywiki-previews/deploys/685a70c15a69940008ec8894
😎 Deploy Preview https://deploy-preview-9101--tiddlywiki-previews.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@saqimtiaz saqimtiaz force-pushed the imagewidget-refresh branch from 7ace65f to 2bb7c79 Compare June 18, 2025 06:01
@saqimtiaz saqimtiaz force-pushed the imagewidget-refresh branch from 48cafd3 to a7434b1 Compare June 18, 2025 06:22
@saqimtiaz saqimtiaz force-pushed the imagewidget-refresh branch from 41260fc to 9d39db7 Compare June 18, 2025 09:24
Copy link
Member

@pmario pmario left a comment

Choose a reason for hiding this comment

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

Since this PR also contains changes to the button-widget, I think the PR title should be adjusted.

@saqimtiaz saqimtiaz force-pushed the imagewidget-refresh branch 2 times, most recently from 089c17f to 5069cc6 Compare June 23, 2025 08:56
@saqimtiaz
Copy link
Member Author

@pmario could you please attempt to engage with the substance of the PR, rather than serving as a syntax checker? The latter is of absolutely no value on a PR clearly described as exploratory and in draft status, whereas actual feedback on the changes explored here would be very helpful.

@saqimtiaz saqimtiaz force-pushed the imagewidget-refresh branch from 5069cc6 to a13279a Compare June 23, 2025 09:01
@saqimtiaz saqimtiaz force-pushed the imagewidget-refresh branch 2 times, most recently from e08e7bd to 205293b Compare June 24, 2025 09:10
@saqimtiaz saqimtiaz changed the title Adds support for data and style attributes in image widget. Refactors widget attribute and refresh handling Jun 24, 2025
@saqimtiaz saqimtiaz moved this to In progress in Planning for v5.4.0 Jun 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In progress
Development

Successfully merging this pull request may close these issues.

2 participants