-
Notifications
You must be signed in to change notification settings - Fork 68
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
Add GitHub-style filter for columns (fix #37) #58
Conversation
The UI may be not perfect because I'm not proficient in that. It's good to do some improvement if you think so. |
Thanks for your detailed comments.
I think so too, but it's backwards incompatible, so that I didn't remove it before just waiting for your opinion.
I think the rising call count is enough to tell users everything goes well. What's more, it's a little tricky to do that: we need to add a fork into data array before we confirm it's indeed useful, so that we have to remove it from the array. The cost is too expensive for me.
There is a misunderstanding. Maybe it's just because I mouse over the inappropriate position.
Certainly I have tested it. I largely agree with you. I hope that this project could be refactored with a modern framework, whether utilizing Vue or React. At the same time a corresponding UI component library would be much helpful. With these, we can simply choose "option 3" after all it's a common feature that we don't need to implement it again. If you think it's OK, maybe I can refactor it in the spare time, using Vue3, Element UI Plus as UI library (because I'm more familiar with them lol) and TypeScript (I love it!). I cannot guarantee a relatively short time frame to begin and finish, but I think it'll be shorter than half a year. And by the way we can also use tools based on React if you think it's better, because I also have a plan to learn it. |
I think it's possible to make a retrocompatible change: after removing the option, you can nonetheless reuse the saved value and use that as the default prefill for the filter field.
Fair enough. (Although I liked how active and dynamic it looked.) I've tested your branch. There are a few bugs that would need to be ironed out:
Comments:
Regarding the rewrite:
Can you point me toward some specific documentation displaying what this common feature would look like?
Then this looks like the perfect opportunity to learn it. ;) |
Something easy will be replied first as following:
In fact I prefer to simply delete it, too.
I have made a simple adjustment to fix them. Do you think the following is satisfying? The bug 1 and 2 are both about "messages". I will try to write a commit named "Fix wrong message display after filtering".
I think the current is better because you may hope that useless forks won't appear in the all process. And we can achieve that.
It recreate the entire table iff the table data is updated (iff the request for a list of forks succeed) or the filter is updated. I'm not sure why it slowed down when moving the mouse around on the rows. I'm sorry but could you confirm that so it is or just a coincidence?
I never use this feature so ignored it before... The button now can download the subset of filtered results. I'm not sure whether the other button is necessary or not, but certainly you can add one if need.
You're right. I have amended it.
The reason lies within the title: Add GitHub-style filter ...
To supplement a prompt in placeholder could be simpler and is also universally used.
Element Plus - Table - filter
Certainly it's all right, if you can accept that I need to spend much more time to finish the job because of learning lol. |
I'm sorry for late response because I'm just too busy this week. Tired and nearly crazy. "Fix wrong message display after filtering" has been committed, which I promised but hadn't finished before.
I agree that it should be positioned under the row "Queried repository". And "between 'Export CSV' and the table" is also not bad for me. (But I almost wanted to cry when I saw how "useful_forks_inject" was built. 😥I haven't changed them because I'm not familiar with JQuery.) But in my opinion, it's reasonable for users to edit the filter field before the searching finishes and at this time the filter should also work.
Done.
My idea: "Export the following table as CSV". The simple is better. Do not increase complexity unnecessarily.
Would you mind providing an example that this feature helps?
I mean, a suitable placeholder is enough and maybe we don't need a filter icon. Certainly it's also OK if added, but I won't do that myself. Certainly I'm convinced that React is powerful enough. After all, developers won't choose a framework if it's so weak. I will try to learn React and use it in |
This is all voluntary. No worries! :) Note: before testing your PR, I merged my latest changes in. They do not affect your logic.
Glad we agree on that.
You introduced a bug. Here's a sequence of events to demonstrate: Only the currently-being-built token should not be treated. The other ones are fine.
Actually a good idea. Although I'd prefer shorter. How about "Export table below as CSV" ?
You can ignore the request. I'll add it myself if I feel motivated enough, I guess.
Same as above.
Great news 👍 |
I make some changes to achieve as below: /**
* 1. Empty filter means no filter.
* 2. Filter string is a list of conditions separated by spaces.
* 3. If a condition is invalid, it is ignored, and the rest of the conditions are applied.
*/
It's actually better. I replaced the string for both the button and the note in settings. |
There are 2 types of filter: `number` and `date`. Tyey both support the following operators: `>`, `<`, `>=`, `<=`, `=`. The format of date should be `yyyy-mm-dd`, like `2022-01-01`. number: `stars`, `forks`, `ahead`, `behind`. date: `pushed` eg: `stars:>0 pushed:>2022-01-01`
I like the rules you opted for. 👍 Three last things:
|
I don't know how to edit $('#useful_forks_inject').append(
$('<div>', {id: UF_ID_WRAPPER}).append(
$('<div>', {id: UF_ID_HEADER}),
$('<div>', {id: UF_ID_MSG}).html(landingPageTrigger()),
$('<div>', {id: UF_ID_DATA}).append(
$('<table>', {id: UF_ID_TABLE}).append(
$('<tbody>')
)
)
)
);
I feel it a bit "no so useful". After all, But it's easy to implement so I can do that if you think it's useful indeed.
It's here: about-searching-on-github However, I think the syntax is simple enough for users to get. |
This should help you understand: https://github.com/useful-forks/useful-forks.github.io/blob/master/website/src/csv-export.js#L15
If it's an easy grab, it's worth it.
I agree, but I like to appease curious minds. I'll try to figure out a way to integrate that link once the PR is merged. |
Sorry I decide to give up. The filter field should be positioned under the msg about searching, but if so then it'll be under the Introducing msg. Different types of msgs occupy the same area. I consider making do is enough for now.
Done. |
This might help you achieve the goal of not always displaying the filter field:
Also, I found another messaging bug: when we input a repo which doesn't exist, the returned message is |
It has been fixed now and I have checked the code relate to msg management to avoid the similar bugs. Well, these works should be finished when taking necessary refactor. But at that time I didn't notice them. |
Thank you. However, it's not my goal. I still think it's reasonable for users to input a filter even if the searching results haven't come. |
This may be fair, but I disagree with the idea that this extra input field should appear at all times. When a new user lands on the home page, it should only see the repository field because that's all they should care about at that point. Another field is just confusing. Again: I'm fine with showing the filter field during the scan, but not before it begins. Nor should it be displayed when there are no useful forks. Essentially, at the earliest it could be displayed once this condition is fulfilled: https://github.com/useful-forks/useful-forks.github.io/blob/master/website/src/queries-logic.js#L316 ... but it'd be even better if it was only displayed when this one is called at least twice (i.e. at least two useful forks are listed): https://github.com/useful-forks/useful-forks.github.io/blob/master/website/src/queries-logic.js#L221 And of course, when any of those 3 messages are displayed, it would make no sense to display that field either: https://github.com/useful-forks/useful-forks.github.io/blob/master/website/src/queries-init.js#L7-L9 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally speaking, this seems to make the code cleaner. I appreciate the changes.
Could you consider extracting that table-related logic and filter-related logic into their own class or files?
That way, queries-logic.js
would remain mostly about the logic handling the GitHub API requests. It's a bit of a particular file because npm run dev
needs to be called to test any change to this file, so it's nice if we can get more code out of it (while also having the great benefit of better encapsulation).
I disabled the automatic formatting function because I found that it will affect almost all the original code. And I'm glad to fix that now because you also noticed that. What's more, the mix of snake case and camel case in function naming exists and it's ignored. When I edit the file, I fail to guess what the difference may mean, and so I just name my functions like the near function. By the way, in my humble opinion, the codebase itself hasn't been something clean, so there is no need to pay too much attention to small changes. Maybe it's only the whole rewrite that can be considerable. |
You seem to have fixed it. Well done! :) I recorded another bug: https://imgur.com/a/2XJRch5 It seems to happen only when the filter is altered while requests are being issued. |
It's undoubtedly a foolish bug. I have fixed it. |
Could you explain to me a how your filtering/comparison code handles dates? |
Please review my latest commits. |
It's just "as is". Before this PR, this project has provided date info as string, like For example, we consider Certainly, if we input |
The current version can not deal with I would like to change the regex to |
Interesting consideration. I think we could get to an even better regex, although definitely more complex.
Now the bummer I just noticed is that as you type The better solution really would be to use |
Gratz! 🥳@Ethkuil Congratulation: your PR made it through the review process. Thanks again for your contribution! ❤️ Fun fact 🤓Here is the final regex in all of its (monstrous) glory:
Gotta love how cryptic and unmaintainable regexes are. |
I want to touch base with you regarding our discussion about rewriting the repository with React.js. After considering my current workload and priorities, I have come to the realization that I won't be able to contribute to this project as much as I had initially hoped. While there is a possibility that I may spend some time on this, the likelihood is low so that you can ignore it for now. (I'll keep you updated if I do decide to work on this.) I apologize for any inconvenience or disappointment this may cause. Thank you for your understanding, and I wish you all the best with the project. Best regards. |
No worries. Like I said: I planned on doing it myself anyways since it's a good learning opportunity. And it's not like you signed any contract. I'm very glad to have collaborated with you. May your talent help other projects move forward. :) For now, my focus will be on #59 since it's pretty high on my priority list. |
There are 2 types of filter:
number
anddate
.Tyey both support the following operators:
>
,<
,>=
,<=
,=
.The format of date should be
yyyy-mm-dd
, like2022-01-01
.number:
stars
,forks
,ahead
,behind
.date:
pushed
eg:
stars:>0 pushed:>2022-01-01
Example screenshot:

I have done tests on
payne911/PieMenu
andWilfred/difftastic
. It worked well. Certainly it would be better if more checks are taken.It's a little troublesome because I'm more familiar with frameworks like Vue that supporting binding data with DOM, but here we have to update DOM ourselves and somewhere code about data and DOM are mixed.
I only did the necessary refactor to make it possible to add the feature of filtering.
And it would be much easier to implement "sort by columns"(#48 ) based on this PR. But I'm not so interested in that 🤔. I would be pleased if anyone would like to do that. Or I can try to solve it in spare time if needed.