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

Expose js function: Spree.SortableTable.refresh #3754

Merged

Conversation

brunoao86
Copy link
Contributor

@brunoao86 brunoao86 commented Sep 4, 2020

Description

The sortable table component is only loaded once when page is loaded.
So it will be very useful to be called after asynchronous requests.

The behaviour stays the same, but now we'll have a function available to refresh the sortable table component if needed by calling Spree.SortableTable.refresh() in the javascript.

Motivation

I'm adding sortable table component on Product / Parts and this exposing was needed.
solidusio-contrib/solidus_product_assembly#92

Checklist:

  • I have followed Pull Request guidelines
  • I have added a detailed description into each commit message
  • I have updated Guides and README accordingly to this change (if needed)
  • I have added tests to cover this change (if needed)
  • I have attached screenshots to this PR for visual changes (if needed)

@spaghetticode
Copy link
Member

@brunoao86 I think this is a useful improvement, thanks for working on it!

I am not 100% sure about the proposed interface Spree.refresh_sortable_tables: I think that it would be better to avoid attaching functions (at least very specific ones) to the global Spree namespace... I would feel more comfortable with something like Spree.SortableTable.refresh(). What do you think?

@brunoao86
Copy link
Contributor Author

@brunoao86 I think this is a useful improvement, thanks for working on it!

I am not 100% sure about the proposed interface Spree.refresh_sortable_tables: I think that it would be better to avoid attaching functions (at least very specific ones) to the global Spree namespace... I would feel more comfortable with something like Spree.SortableTable.refresh(). What do you think?

Thanks for the review, @spaghetticode! It makes total sense, I will work on that!

@brunoao86 brunoao86 changed the title Expose js function: Spree.refresh_sortable_tables Expose js function: Spree.SortableTable.refresh Sep 8, 2020
brunoao86 added a commit to penrosehill/solidus that referenced this pull request Sep 9, 2020
from Spree.refresh_sortable_tables
to   Spree.SortableTable.refresh

Reference: solidusio#3754 (comment)
brunoao86 added a commit to penrosehill/solidus_product_assembly that referenced this pull request Sep 9, 2020
@brunoao86
Copy link
Contributor Author

@brunoao86 I think this is a useful improvement, thanks for working on it!

I am not 100% sure about the proposed interface Spree.refresh_sortable_tables: I think that it would be better to avoid attaching functions (at least very specific ones) to the global Spree namespace... I would feel more comfortable with something like Spree.SortableTable.refresh(). What do you think?

Hey @spaghetticode! ✌️
Hope all is well.

When you have some free time, can you please review this PR again?
I applied your suggestion on this commit fb4f841

Thanks again! 🙏

spaghetticode
spaghetticode previously approved these changes Sep 17, 2020
Copy link
Member

@spaghetticode spaghetticode left a comment

Choose a reason for hiding this comment

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

Looks good, can you please squash the commits into a single one? thanks

@spaghetticode spaghetticode self-requested a review September 17, 2020 15:38
@spaghetticode spaghetticode dismissed their stale review September 17, 2020 15:40

approved by mistake

@spaghetticode
Copy link
Member

@brunoao86 sorry, I just realized the changes are scattered in 3 commits. Can you squash them into a single one, with a good commit message? thanks 🙏

The sortable table component is only loaded once when page is loaded.
So it will be very useful to be called after asynchronous requests.

Motivation: To allow sorting products inside a product assembly (Product Parts)
solidusio-contrib/solidus_product_assembly#92
@brunoao86 brunoao86 force-pushed the js-expose-refresh-sortable-tables branch from 98b70a8 to d46d1bc Compare September 17, 2020 18:11
@brunoao86
Copy link
Contributor Author

Hey @spaghetticode

It is done! ✔️

Is there a way to do the squash without the force-pushed? I just did that when merging the code so I don't know if I did it right.

Thanks! 🙏

Copy link
Member

@spaghetticode spaghetticode left a comment

Choose a reason for hiding this comment

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

@brunoao86 push -f is ok 👍
Again, thanks for this contribution 🎉

@aldesantis aldesantis merged commit 3c6f339 into solidusio:master Sep 18, 2020
@aldesantis
Copy link
Member

Thanks @brunoao86!

@brunoao86 brunoao86 deleted the js-expose-refresh-sortable-tables branch September 18, 2020 14:08
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.

3 participants