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

Fix: Add _classes to hotgrid__item (fixes #143) #144

Merged
merged 4 commits into from
Jan 10, 2024
Merged

Conversation

swashbuck
Copy link
Contributor

@swashbuck swashbuck commented Jan 9, 2024

Fix #143

Fix

  • Adds the _items._classes to .hotgrid__item to assist with styling.
  • Retains the custom class on the Hotgrid popups.

@swashbuck swashbuck self-assigned this Jan 9, 2024
@oliverfoster
Copy link
Member

Should probably put it on .hotgrid__item and not .hotgrid__item-btn

@swashbuck swashbuck changed the title Fix: Add _classes to hotgrid__item-btn (fixes #143) Fix: Add _classes to hotgrid__item (fixes #143) Jan 9, 2024
@swashbuck
Copy link
Contributor Author

Should probably put it on .hotgrid__item and not .hotgrid__item-btn

Yep, done. I initially had it on hotgrid__item-btn since that's where the other classes were (is-round, is-visited, etc).

@oliverfoster
Copy link
Member

Yep, done. I initially had it on hotgrid__item-btn since that's where the other classes were (is-round, is-visited, etc).

Yea it's curious.

@kirsty-hames
Copy link
Contributor

Yep, done. I initially had it on hotgrid__item-btn since that's where the other classes were (is-round, is-visited, etc).

Yea it's curious.

I think Hotgrid was based on Hotgraphic initially so this has probably carried over. Hotgraphic appends _classes and state classes to the button (.hotgraphic__pin / .hotgraphic__tile).

Copy link
Contributor

@kirsty-hames kirsty-hames left a comment

Choose a reason for hiding this comment

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

This works as expected thanks @swashbuck. One thing to note, Hotgraphic has separate _classes for popup and item. Would we want to do the same here for consistency?

Although I do find it odd that the Hotgraphic popup _classes is associated with the graphic when the class is applied to the .hotgraphic-popup__item.

@swashbuck
Copy link
Contributor Author

This works as expected thanks @swashbuck. One thing to note, Hotgraphic has separate _classes for popup and item. Would we want to do the same here for consistency?

Although I do find it odd that the Hotgraphic popup _classes is associated with the graphic when the class is applied to the .hotgraphic-popup__item.

Thanks, @kirsty-hames . I agree that we should align with Hotgraphic's classes implementation, as flawed as it is. I'll add a new _classes property to each Hotgrid item's _graphic object. Then, maybe in the future, we can look into changing both Hotgraphic and Hotgrid's implementation so that it makes more sense.

@swashbuck
Copy link
Contributor Author

@kirsty-hames @oliverfoster These changes have been made. Ready for re-review.

Copy link
Contributor

@kirsty-hames kirsty-hames left a comment

Choose a reason for hiding this comment

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

Thanks @swashbuck - works as expected.

@swashbuck swashbuck merged commit 22787b1 into master Jan 10, 2024
@swashbuck swashbuck deleted the issue/143 branch January 10, 2024 17:34
Copy link

🎉 This PR is included in version 4.5.1 🎉

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.

Add _items._classes to the button element
3 participants