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

Event onReady #79

Merged
merged 5 commits into from
Sep 4, 2024
Merged

Event onReady #79

merged 5 commits into from
Sep 4, 2024

Conversation

frenicohansen
Copy link
Contributor

@frenicohansen frenicohansen commented Sep 1, 2024

Background

As I was working with the library, I encountered a function called onPositionComplete. According to its documentation, this function was meant to "add a listener used to execute a function once all items in the grid have been properly positioned." However, something about the name and the description felt off. It gave the impression that the listener would only be triggered a single time, likely on the first positioning of the items in the grid.

As I dug deeper into the code, it became clear that onPositionComplete actually behaved differently than what the name and documentation suggested. The listener wasn't just a one-time thing—it would trigger every time the positionItems() method was called or when the container was resized. This behavior was useful, but the existing naming and documentation didn't make that clear.

Changes

  • Introduced onReady to run a listener only once, after the first positioning of items.
    • This is useful for scenarios like managing a loading state, where you only want to take action once the grid is properly positioned for the first time, as mentioned in Complete positionItems callback? #56.
  • Fixed the return type of the original onPositionComplete from void to number to match its actual behavior.
  • Updated documentation for onPositionComplete and onGridReady

@e-oj
Copy link
Owner

e-oj commented Sep 2, 2024

Nice idea! I think we can just emit a ready event in the listen function, add an onReady function, and leave everything else the same. That way there won't be any breaking changes with this feature.

The current setup, where positionComplete is emitted whenever positionItems is called, makes sense to me so I don't think we have to add an onRepositionComplete

@e-oj
Copy link
Owner

e-oj commented Sep 2, 2024

Also you're right about the documentation; we should update that to make it clearer as well. Thanks for pointing this out!

@frenicohansen
Copy link
Contributor Author

Nice idea! I think we can just emit a ready event in the listen function, add an onReady function, and leave everything else the same. That way there won't be any breaking changes with this feature.

The current setup, where positionComplete is emitted whenever positionItems is called, makes sense to me so I don't think we have to add an onRepositionComplete

I updated the PR description and implemented your suggestion. With this there should be no breaking changes.

@frenicohansen frenicohansen changed the title Events Listener onPositionComplete and onRepositionComplete Events Listener onReady Sep 3, 2024
@frenicohansen frenicohansen changed the title Events Listener onReady Event onReady Sep 3, 2024
/**
* Adds a callback for when positioning is complete
* @param callback - function to be called on positioning complete
*/
onPositionComplete(callback: () => void): void;
onPositionComplete(callback: () => void): number;
Copy link
Owner

Choose a reason for hiding this comment

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

Nice catch!

README.md Outdated Show resolved Hide resolved
@e-oj
Copy link
Owner

e-oj commented Sep 4, 2024

Awesome work. Thanks!

@e-oj e-oj merged commit 1c373cb into e-oj:master Sep 4, 2024
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.

2 participants