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

Fixes #120: revamp JavaScript on the download page #123

Merged
merged 3 commits into from
Jun 2, 2020

Conversation

sorawee
Copy link
Contributor

@sorawee sorawee commented Jun 1, 2020

This PR fixes three bugs:

  • Remove an extra "Variant" widget.
  • Remove redundant "Platform" options
  • Add extension information to variants

Additionally, it replaces the current implementation to a declarative approach
similar to big-bang / React framework. The current approach is extremely
complicated due to its imperative nature, generating multiple selection widgets,
embedding several information in non-standard attributes of DOM elements,
and showing/hiding them via JS. This is very error prone due to multiple levels
of the selection widgets, and how the number of levels is dynamic. Additionally,
it is very difficult to modify this page.

The new approach is similar to big-bang. We supply the framework the initial
state and the toDraw function, which constructs the DOM tree based on the input
state. Each DOM element can have a handler (such as onclick) which will
transition the current state into the next state. This makes it much easier to
reason about it.

This PR fixes three bugs:
- Remove an extra "Variant" widget.
- Remove redundant "Platform" options
- Add extension information to variants

Additionally, it replaces the current implementation to a declarative approach
similar to big-bang / React framework. The current approach is _extremely_
complicated due to its imperative nature, generating multiple selection widgets,
embedding several information in non-standard attributes of DOM elements,
and showing/hiding them via JS. This is very error prone due to multiple levels
of the selection widgets, and how the number of levels is dynamic. Additionally,
it is very difficult to modify this page.

The new approach is similar to big-bang. We supply the framework the initial
state and the toDraw function, which constructs the DOM tree based on the input
state. Each DOM element can have a handler (such as onclick) which will
transition the current state into the next state. This makes it much easier to
reason about it.
@sorawee
Copy link
Contributor Author

sorawee commented Jun 1, 2020

Before:
Screen Shot 2020-06-01 at 14 04 53

After:
Screen Shot 2020-06-01 at 14 05 03


Before:
Screen Shot 2020-06-01 at 14 04 40

After:
Screen Shot 2020-06-01 at 14 04 30


Before:
Screen Shot 2020-06-01 at 14 04 19

After:
Screen Shot 2020-06-01 at 14 03 57

@sorawee
Copy link
Contributor Author

sorawee commented Jun 1, 2020

Also note that rolled my own big-bang and didn't use React (even though React is really what we should be using) because it doesn't support very old browsers. My big-bang implementation only uses standard JS features which should support IE8 at the very least (and I think it should support IE6 too).

@mflatt
Copy link
Member

mflatt commented Jun 1, 2020

This works ok for me so far, and I can easily believe that it's better code.

Why the "(dmg)" or "(exe)" or "(sh)" in the "Variant" menu? Since that file extension is shown in the link immediately below, it seems redundant.

@sorawee
Copy link
Contributor Author

sorawee commented Jun 1, 2020

See the last before-after comparison. While it's true that the extension is clear when it's already chosen, it's not clear when it's about to be chosen. That means users need to guess which one is the right one, and if they guess wrong, they have to try again.

@mflatt
Copy link
Member

mflatt commented Jun 1, 2020

Ah, I see. But the "(dmg)" or "(exe)" is noisy most of the time, when all the choices have the same suffix. Also, "(exe)" vs. "(tgz)" are not different "variants" in the sense of BC vs. CS, so it doesn't seem like the right place for that choice.

The old intent was that the "tgz" option wouldn't show up in the menus, and it was instead left to "More Variants and Checksums" (but "variant" is perhaps not the right word anymore if it's going to be BC vs. CS), and that still seems better to me.

@sorawee
Copy link
Contributor Author

sorawee commented Jun 1, 2020

OK, I can fix that tonight. With the new framework, this should be easy to do.

@sorawee
Copy link
Contributor Author

sorawee commented Jun 1, 2020

Just to be clear, my plan is to add another level of selection widget named "Extension" when there are multiple extensions. If you have a better idea, let me know!

@mflatt
Copy link
Member

mflatt commented Jun 1, 2020

"Extension" is not a good name, because it's not a choice about a file extension. If there's going to be a choice available, the presentation will need more thought.

The choice is between (variously) an installer or disk image and a tarball. "Installer" seems clear enough. "Disk image" is accurate for Mac, but I'm not sure that's terminology that a novice Mac user will recognize, so maybe "installer" is a better approximation. And maybe "archive" is a good word for the second option? Maybe, also, there should be some explanatory text that show up for the "archive" option — something that says it's a directory/folder that can be unpacked and run in place.

But I still think it's better to omit the choice. Especially since there are other distribution options that are not covered by these choices, it's simpler to leave all of the non-obvious ones to "More Varians and Checksums".

@mflatt
Copy link
Member

mflatt commented Jun 1, 2020

What would be a better word than "variant" (which is generic and overloaded) for the "Regular" vs. "CS" choice? It's a choice of runtime systems, but "Runtime" or "VM" would be too obscure for most users.

@sorawee
Copy link
Contributor Author

sorawee commented Jun 1, 2020

I think we should make "Variant" means "Regular" vs "CS". The documentation has been using "Variant" in this sense.

"More Variants and Checksums" then could be changed to "More Installers and Checksums".

@mflatt
Copy link
Member

mflatt commented Jun 1, 2020

Ok, changing to "More Installers and Checksums" makes sense to me.

@sorawee
Copy link
Contributor Author

sorawee commented Jun 1, 2020

Especially since there are other distribution options that are not covered by these choices

I'm not aware of distribution options that are not covered by the current interface. Could you give me some examples?

My initial thought is that because the selection widgets are now much easier to manipulate, showing all installers in the current interface, defaulting to the "obvious ones" is easily doable, and it makes it easier to discover "non-obvious ones". Following this thought, "More Variants/Installers and Checksums" would be changed to simply "Checksums".

@mflatt
Copy link
Member

mflatt commented Jun 1, 2020

I think there's currently no way to get to the "natipkg" installers.

I'll yield to other opinions, but "More Installers and Checksums" seems to me a much more discoverable presentation. Maybe a useful direction is to make "More Installers and Checksums" itself more discoverable.

@sorawee
Copy link
Contributor Author

sorawee commented Jun 2, 2020

Done. It turns out that the situation where there could be more than one installers for each variant is when they are exe and tgz, so for these cases, I added a filter to discard anything that is not exe out.

Also renamed the link to "More Installers and Checksums".

@mflatt
Copy link
Member

mflatt commented Jun 2, 2020

This looks good to me, so I'm happy to merge.

Instead of filtering the extra tgzs, though, it occurs to me that maybe the problem is that they shouldn't have been listed in installers.txt in the first place. @jbclements - any opinion on that?

@mflatt mflatt merged commit 4912e06 into racket:master Jun 2, 2020
@sorawee sorawee deleted the fix-120 branch June 3, 2020 00:41
@jbclements
Copy link
Contributor

Thanks for the bump. I filter the installers using a program called filter-installers.rkt, which I see is non-public because it's in the release-pltbuild repo rather than in the racket-lang-org repo. It could easily live there instead. A quick look suggests that I could maybe just drop it in the download directory, if that makes sense to you.

...

Okay, I've done that. Also, I see that there was a bug, in that the file was looking for files containing "windows" rather than "win32". Mea culpa.

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

3 participants