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

Marktplaats categories added #3761

Merged
merged 5 commits into from
Oct 22, 2023

Conversation

Park0
Copy link
Contributor

@Park0 Park0 commented Oct 15, 2023

Hopefully fixes #3613
Haven't tested it. I can only that the l2 category is added to the query string. It may be required to also add the l1 category.

Maybe @Pizzabroodje can test that for me :)

@github-actions
Copy link

github-actions bot commented Oct 15, 2023

Pull request artifacts

Bridge Context Status
Marktplaats 1 Search (current) ⚠️ The feed has no items
Marktplaats 1 Search (pr) ⚠️ The feed has no items

last change: Sunday 2023-10-22 15:34:58

@dvikan
Copy link
Contributor

dvikan commented Oct 16, 2023

this gonna massively increase the html on frontpage where all bridges are rendered. dont like it

As the whole list is too big only main categories are used for now.
@Park0
Copy link
Contributor Author

Park0 commented Oct 17, 2023

I removed the sub categories. I just copied them in. I did not see it got so big :).

@Pizzabroodje
Copy link

I removed the sub categories. I just copied them in. I did not see it got so big :).

Would there maybe be some way to dynamically load the L2 categories based on the L1 category that's selected? I'm guessing not as they're loaded into a constant?

@Park0
Copy link
Contributor Author

Park0 commented Oct 18, 2023

I removed the sub categories. I just copied them in. I did not see it got so big :).

Would there maybe be some way to dynamically load the L2 categories based on the L1 category that's selected? I'm guessing not as they're loaded into a constant?

That would require some JS dependencies. If that would be acceptable than i could add that.

Renamed unused method to better reflect it usage
@Park0
Copy link
Contributor Author

Park0 commented Oct 18, 2023

Done both

@Park0
Copy link
Contributor Author

Park0 commented Oct 18, 2023

I removed the sub categories. I just copied them in. I did not see it got so big :).

Would there maybe be some way to dynamically load the L2 categories based on the L1 category that's selected? I'm guessing not as they're loaded into a constant?

@Pizzabroodje i think u better can be a separated feature request for that

@Pizzabroodje
Copy link

Pizzabroodje commented Oct 18, 2023

I removed the sub categories. I just copied them in. I did not see it got so big :).

Would there maybe be some way to dynamically load the L2 categories based on the L1 category that's selected? I'm guessing not as they're loaded into a constant?

@Pizzabroodje i think u better can be a separated feature request for that

Alright I'll do that.

The L1 categories can be dynamically loaded using something like this in front of the class declaration (this puts <optgroup label="0"> around the options though which I don't know where that comes from). I guess this would also slow the front page down though:

function getCats() {
	$marktplaatsHTML = file_get_html("https://www.marktplaats.nl");
	
	$categories = array();
	
	foreach($marktplaatsHTML->find("select[id=categoryId] option") as $opt) {
		if(!str_contains($opt->innertext, "categorie")) {
			$categories += [$opt->innertext => $opt->value];
		}
	}
	
	return $categories;
}

define('CATS', getCats());

I don't know if Marktplaats categories ever change, but I don't think I've ever seen that happen in the 10+ years of using it. So not too important anyways. I think hardcoding the L1 categories along with a JS dependency to dynamically load the L2 categories would be best. Then it'll only do a request when selecting a category and thus wouldn't slow down the front page.

@Pizzabroodje
Copy link

Pizzabroodje commented Oct 18, 2023

@Park0
I wanted to use the one with hardcoded L2 categories (the first commit) myself, but get this error:
image
Using the newer commits I don't get that.

Nevermind, fixed it. Was an issue with some square brackets not being closed properly.

@Pizzabroodje
Copy link

Pizzabroodje commented Oct 18, 2023

Hopefully fixes #3613 Haven't tested it. I can only that the l2 category is added to the query string. It may be required to also add the l1 category.

Maybe @Pizzabroodje can test that for me :)

Just tested this; L1 category is indeed required along with the L2 category. Only a L1 category works fine, but only a L2 category does not.

@Pizzabroodje
Copy link

Pizzabroodje commented Oct 18, 2023

@Park0
Also, what didn't work with the 'Fietsen en Brommers' category? For me it works fine. This is actually the category (and the sub-category 'Mountainbikes and ATB' along with it) that I used for my personal bridge a while back.

For anyone stumbling upon this thread that wants to search within a L2 category, I modified the first commit a little bit to fix some things, and added the 'Fietsen en Brommers' categories:
MarktplaatsBridge.txt

@Park0
Copy link
Contributor Author

Park0 commented Oct 19, 2023

  • The error about the fietsen category was for the scraping function, It may already work.
  • The categories in rss-bridge should be static in any case. The scraping of the main categories could be added to the function.
  • The error was a coding issue with the array but i think you already found that.
  • Good to know. The code now has both.
  • It was a issue with the scraping not sure what caused it. I may try it when i have some time.

Categories completed
Added a default empty one
Check if the input is not empty before using
Added helper methods to generate the categorylist
@Park0
Copy link
Contributor Author

Park0 commented Oct 20, 2023

I made some small fixes. I am not aware of any issues on the current code. @dvikan if your happy with the code it can be merged.

@Park0 Park0 requested a review from dvikan October 21, 2023 13:43
@dvikan
Copy link
Contributor

dvikan commented Oct 21, 2023

ci fail

@dvikan
Copy link
Contributor

dvikan commented Oct 21, 2023

sorry about the ci being unclear.

it complains about public functions that should not be there.

make the helper functions non-public and it should be resolved.

Set the methods to private for the CI
@Park0
Copy link
Contributor Author

Park0 commented Oct 22, 2023

@dvikan Now it seems happy :)

@dvikan dvikan merged commit f134808 into RSS-Bridge:master Oct 22, 2023
7 checks passed
@dvikan
Copy link
Contributor

dvikan commented Oct 22, 2023

rssbridge isnt setup nicely to handle much js stuff. also i dont want to maintain js. but we can revisit topic later

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.

Additional filters for Marktplaats bridge
3 participants