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

New: Add negative transform to hotgraphic pin (fixes #299) #300

Merged
merged 3 commits into from
May 3, 2024

Conversation

guywillis
Copy link
Contributor

@guywillis guywillis commented Mar 6, 2024

Fixes: #299

New

  • Added config which, when enabled, adds a negative transform to hotgraphic pin so it remains stationary when viewing responsively

@guywillis guywillis added the bug label Mar 6, 2024
@guywillis guywillis self-assigned this Mar 6, 2024
@oliverfoster
Copy link
Member

Is every pin image going to need to be centered always?

@guywillis
Copy link
Contributor Author

I've excluded pin images from the amend and instead only targeted pins.

The proposed amend does not force pins to be centred on the hotgraphic image, I am only proposing that their transform origin is moved from top left to center.

The screenshots in the issue are only for illustration purpose to show the difference.

@oliverfoster
Copy link
Member

oliverfoster commented Mar 6, 2024

I am only proposing that their transform origin is moved from top left to center.

Yes.

Is every pin image going to need to be centered always?

Should this be a theme thing? Or selectable or with classes?

Note: Course creators will likely have to amend the position of hotgraphic pins when updating their plugin as this fix will introduce an offset due to the element origin moving from top left to centre.

You acknowledge it's breaking on the issue. Please change the pr status to breaking or revise.

@guywillis
Copy link
Contributor Author

Is every pin image going to need to be centered always?

Certainly makes for more consistency then the current implementation.

Should this be a theme thing? Or selectable or with classes?

I personally think it should be in by default. It could be made as part of the theme and / or custom classes but, to me, that's putting obstacles in between the user and the fix.

You acknowledge it's breaking on the issue. Please change the pr status to breaking or revise.

Didn't realise it would be classified as breaking. Will update issue.

@guywillis guywillis changed the title Fix: Add negative transform to hotgraphic pin (fixes #299) Breaking: Add negative transform to hotgraphic pin (fixes #299) Mar 6, 2024
@oliverfoster
Copy link
Member

oliverfoster commented Mar 6, 2024

Breaking is when you change something that requires other people to rework their stuff.

There is a potential that many well placed pins, on many courses, on many hotgraphics in any number of AATs will now change. That's a lot of work created by one simple transform.

@swashbuck
Copy link
Contributor

swashbuck commented Mar 18, 2024

Sort of like what Dan was saying, what if this was a new configurable option so that it doesn't break older courses?

_transformOrigin: top left (default) or center

Or if we want 'center' to be the default (which does make more sense), this could be an improvement that we implement once the migration scripts are ready. Older courses would be migrated to set this value to "top left".

@kirsty-hames
Copy link
Contributor

This is something I've implemented in custom themes many times so would be good to fix the issue in the plugin itself. To avoid a breaking change I'd be incline to go with @swashbucks suggestion above.

@oliverfoster
Copy link
Member

Could ya'll approve if in agreement please?

@kirsty-hames
Copy link
Contributor

Could ya'll approve if in agreement please?

@guywillis, are you happy to make this a config option as per above?

@guywillis
Copy link
Contributor Author

@kirsty-hames Of course.

Whilst it would be nice to have the option included by default I do understand and appreciate the impact of doing so is potentially huge.

@guywillis guywillis changed the title Breaking: Add negative transform to hotgraphic pin (fixes #299) New: Add negative transform to hotgraphic pin (fixes #299) Apr 30, 2024
@swashbuck
Copy link
Contributor

I've excluded pin images from the amend and instead only targeted pins.

@guywillis What was the rationale for excluding pin images? Is the transform origin already centered?

@guywillis
Copy link
Contributor Author

I've excluded pin images from the amend and instead only targeted pins.

@guywillis What was the rationale for excluding pin images? Is the transform origin already centered?

Nothing more than a misunderstanding between pin images and tiles. I shall correct, good spot.

Copy link
Contributor

Choose a reason for hiding this comment

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

👀

@cahirodoherty-learningpool cahirodoherty-learningpool merged commit 7bcc696 into master May 3, 2024
github-actions bot pushed a commit that referenced this pull request May 3, 2024
# [6.12.0](v6.11.3...v6.12.0) (2024-05-03)

### New

* Add negative transform to hotgraphic pin (fixes #299) (#300) ([7bcc696](7bcc696)), closes [#299](#299) [#300](#300)
Copy link

github-actions bot commented May 3, 2024

🎉 This PR is included in version 6.12.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

Hotgraphic pin is not static responsively
5 participants