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

WIP: Feature/add #6077

Closed
wants to merge 1 commit into from
Closed

WIP: Feature/add #6077

wants to merge 1 commit into from

Conversation

wenzhixin
Copy link
Owner

@wenzhixin wenzhixin commented Mar 12, 2022

🤔Type of Request

  • Bug fix
  • New feature
  • Improvement
  • Documentation
  • Other

🔗Resolves an issue?
Fix #6058

📝Changelog

  • Added update sortable support of updateColumnTitle method.
  • Core
  • Extensions

💡Example(s)?
Before: https://live.bootstrap-table.com/code/antonioaltamura/10565
After: https://live.bootstrap-table.com/code/wenzhixin/10788

☑️Self Check before Merge

⚠️ Please check all items below before review. ⚠️

  • Doc is updated/provided or not needed
  • Demo is updated/provided or not needed
  • Changelog is provided or not needed

Copy link
Collaborator

@UtechtDustin UtechtDustin left a comment

Choose a reason for hiding this comment

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

I don't think it makes sense that you can update the sortable option via method updateColumnTitle.
As the method name already says update column TITLE.

Then we have a method which name says update title but also could update the sortable option.

@wenzhixin
Copy link
Owner Author

Do you think add a new method like updateColumnSortable?

@UtechtDustin
Copy link
Collaborator

I guess a method updateColumn or updateColumnOptions makes more sense. so we can implement mehr stuff.
But i wonder that we don't have a method like this.

$el.removeClass('sortable both asc desc')
}
}
$(el).data(column)
Copy link
Collaborator

Choose a reason for hiding this comment

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

we're instantiating a lot of jquery object with $(el). I think we could use something like const element = $(el);

@wenzhixin wenzhixin changed the title Feature/add WIP: Feature/add Mar 14, 2022
@UtechtDustin
Copy link
Collaborator

Any news here @wenzhixin ?

@wenzhixin
Copy link
Owner Author

But i wonder that we don't have a method like this.

I think it's okay to set it up directly with the refreshOptions method.

So let's close this PR?

@djhvscf
Copy link
Collaborator

djhvscf commented May 14, 2022

We can either change the method name or implement a new method where we can change all the column config

@UtechtDustin
Copy link
Collaborator

I think we should add a new method which allowes to change all column options as @djhvscf said, but i guess this has no high priority yet.
So the "workaround" using the refreshOptions is ok for now.

@UtechtDustin UtechtDustin deleted the feature/add branch May 14, 2022 17:57
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.

Unable to set sortable flag via updateColumnTitle()
3 participants