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

PR for Issues (https://github.com/adazzle/react-data-grid/issues/1441, https://github.com/adazzle/react-data-grid/issues/1442, https://github.com/adazzle/react-data-grid/issues/1400, https://github.com/adazzle/react-data-grid/issues/1181, https://github.com/adazzle/react-data-grid/issues/1443) #1449

Closed
wants to merge 18 commits into from

Conversation

madhan-dhamodaran
Copy link

@madhan-dhamodaran madhan-dhamodaran commented Jan 10, 2019

Description

A few sentences describing the overall goals of the pull request's commits.

Please check if the PR fulfills these requirements

What kind of change does this PR introduce? (check one with "x")

[ ] Bugfix
[ X] Feature
[ ] Code style update (formatting, local variables)
[ X] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Other... Please describe:

What is the current behavior? (You can also link to an open issue here)

What is the new behavior?

Does this PR introduce a breaking change? (check one with "x")

[ ] Yes
[ ] No

If this PR contains a breaking change, please describe the impact and migration path for existing applications: ...

Other information:

@madhan-dhamodaran
Copy link
Author

I will be working on resolving the conflicts and meeting the other requirements.

Thanks,
Madhan

@@ -141,6 +141,13 @@ class Grid extends React.Component {
this.emptyView = emptyView;
};

getHeaderScroll = () => {
if(typeof(this.props.enableHeaderScroll) === 'undefined' || this.props.enableHeaderScroll) {
Copy link
Author

Choose a reason for hiding this comment

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

This fixes the issue when you are on right most of the grid and try to scroll down on a filter dropdown using the scroll bar, it pushes the user to the left most of the grid.

@madhan-dhamodaran
Copy link
Author

@amanmahajan7 @jlbooker @Jcholerton-Adazzle ,

Hi All, this is a major change and I have introduced a lot of features to the grid. Please have a review and let me know if you have any comments.

Thanks,
Madhan

@madhan-dhamodaran
Copy link
Author

@amanmahajan7 @jlbooker @Jcholerton-Adazzle , @alefherrera @tenpaiyomi @azu @maiis @txm @LittleBoxOfSunshine , Was anyone able to look into this PR pls.

@richardware
Copy link
Contributor

Hi @madhan-dhamodaran,

Sorry you haven't yet had any feedback on this.

It all looks interesting, but it's very difficult to review in the current format - is there a reason why this couldn't be split in to one PR per issue?

@madhan-dhamodaran
Copy link
Author

hi @richardware ,

Sorry for the late reply. This is worked for almost the whole last year and I incorrectly created a branch out from my fork and had individuals PRs raised and realized they were going to be merged only to my fork. So, when I merged it, everything just formed a huge change list. So here we are now. However, I have introduced a lot of features to the grid. So, if someone could review this and take it to next step, it will be really helpful. Sorry again for the huge number of changes in just one PR. Kindly do the needful.

Thanks,
Madhan

@l-wagner
Copy link

l-wagner commented Feb 6, 2019

To show some user interest: The typeahead filter, the tooltip headers and the react-select upgrade are features/improvements that would make my life a lot easier. I understand that it's a vast amount of changes to review, but I'd very much appreciate it. :)

@madhan-dhamodaran
Copy link
Author

@amanmahajan7 @jlbooker @Jcholerton-Adazzle , @alefherrera @tenpaiyomi @azu @maiis @txm @LittleBoxOfSunshine @richardware , Hey guys, was anyone able to look in to this?

Kindly advise.

Thanks,
Madhan

@amanmahajan7
Copy link
Contributor

Hi @madhan-dhamodaran thank you so much for the PR and apologizes for the late reply. Unfortunately this PR cannot be reviewed in its current state and we need to break it down into separate PRs so things can be reverted/fixed in isolation.

We are also considering retiring some of the existing addons and instead focus on making RDG more flexible so users can customize it however they like. There are a few reasons behind this decision

  • Addons are not actively maintained
  • It is impossible to handle every use case

#1441 Typeahead Filter is great but as I mentioned previously it will be hard to address every use case. I would recommend adding this filter to a new repository and publish it separately

#1442 Add/Remove Columns another great feature but I think it does not need to part of core RDG repository

#1181 - Same reasoning

#1400 - Will accept PR

#1443 - set default filtering values in filter text boxes Will accept PR

Thank you again

@dsadaka
Copy link

dsadaka commented Mar 19, 2019

Hello everyone. I am tasked with adding saved filters from cookies. I read about PR 1443 and it looks like exactly what I need. I see from above that it was accepted but, as of this writing, is still open. We're about to upgrade the grid to the latest version and are looking forward to this feature (along with the fix to the checkbox column when filtering).

Can anyone please provide a status for 1443?

@BlaineBradbury
Copy link

@amanmahajan7 are you able to review this one? Especially for the #1400 bug.

@starkysclub
Copy link

Hey guys I separated the #1400 bug to a new pull request, so it can get merged more quickly, can you look into it?

Base automatically changed from master to main January 19, 2021 20:20
@amanmahajan7
Copy link
Contributor

Closing this PR as we have decided to keep the react-grid-code slim so all the filters/editors should be implemented externally.

https://github.com/adazzle/react-data-grid/blob/canary/CHANGELOG.md
https://adazzle.github.io/react-data-grid/canary/?path=/story/demos--common-features
https://github.com/adazzle/react-data-grid/tree/canary/stories/demos

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.

None yet

7 participants