Refactored console-tabs.js

[adamzap]

• Stopped using jQuery
• Moved refactored code to djangoproject.js

I was also to remove the content showing/hiding functionality that was in the original file because it's handled in CSS. Is there a reason this was in the original JavaScript?

The elements on the right side of the screenshot below are affected by this change:

[pauloxnet]

I was also to remove the content showing/hiding functionality that was in the original file because it's handled in CSS. Is there a reason this was in the original JavaScript?

For me the more JavaScript you can remove in favour of CSS, the better.

[bmispelon]

Haven't tested it yet, but it looks quite good from the get go (as usual).

Is there a reason this was in the original JavaScript?

I haven't dug in the git history, but I would assume that things got first implemented in javascript, and the js code stuck around when the show/hide behavior was reimplemented in CSS.

Do you have a quick way to test this locally, or does one have to go and build the docs and check there?

[adamzap]

@bmispelon I test all of my PRs locally, and it seems fine to me. I hope that someone else can try it, but I think you will need to build the docs. Does this project have a staging server you can deploy the branch to as an alternative to building the docs locally?

[adamzap]

@bmispelon Both the CSS and JavaScript approaches seem to have been added in 8bcedbe. I don't understand why yet, and that's part of the reason I'm hoping that someone can help me test this. Neither approach has been meaningfully changed since being added.

[bmispelon]

I've tested the new js locally and it appears to work just as well as before 👍🏻

I'm not sure it's necessary, but I've added a suggestion to make the js a bit more flexible in case more tabs get added in the future (that might not really be realistic).

Curious what you think, but if you don't think that's a good idea I'm more than happy to merge this as-is.

[bmispelon]

Not sure if it's worth the added complexity, but a regexp could be used to not have to hardcode unix and win. What do you think?

Suggested change

	const input_id = e.currentTarget.getAttribute('for');
	const selector = input_id.endsWith('unix') ? '.c-tab-unix' : '.c-tab-win';
	const input_id = e.currentTarget.getAttribute('for');
	const input_id_regex = /^c-tab-\d+-(?<platform>[a-z]+)$/;
	const platform = input_id.match(input_id_regex).groups.platform;
	const selector = `.c-tab-${platform}`;

[adamzap]

It's a fair question. As you said, it adds complexity in the form of a small feature that didn't exist in the old code, and I'm not trying to add features in these PRs. However, I'm happy to make the change if you think it's useful, but I'd probably be a -0 at this point.

I may be in favor of a simpler implementation:

    const input_id = e.currentTarget.getAttribute('for');
    const selector = '.c-tab-' + input_id.split('-').at(-1);

Maybe this way of doing it is a good balance: not too complex, but it will also support possible future platforms as long as they don't contain a dash.

What is your preference between the three implementation options?

[bmispelon]

@bmispelon Both the CSS and JavaScript approaches seem to have been added in 8bcedbe. I don't understand why yet, and that's part of the reason I'm hoping that someone can help me test this. Neither approach has been meaningfully changed since being added.

Thanks for digging. I also don't really understand how things came together. Docs stuff can be in a weird place because the HTML gets generated by sphinx based on the configuration in the django/django repository, but the CSS (and js) is managed on this repo. So I could imagine things getting added at different stages and not realizing that the js is doing double work with the CSS for example.

But either way, I think your changes are good and I don't see a good reason to keep the double logic. CSS is the way to go!