Skip to content

V5 - complete rewrite #404

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

Merged
merged 2 commits into from
May 8, 2021
Merged

V5 - complete rewrite #404

merged 2 commits into from
May 8, 2021

Conversation

sushain97
Copy link
Member

@sushain97 sushain97 commented May 1, 2021

This PR is a complete rewrite of apertium-html-tools in React + TypeScript with an ESBuild + Node build toolchain. Fixes #351. Pretty sure I also fixed #384, #373, #357, #266 and more along the way.

This is pulled from https://github.com/sushain97/apertium-html-tools-v2 where you can easily browse the source. Before I merge this PR, I will pull in any changes from that repo.

For your ease of testing, I've hosted a version here: https://azurite.skc.name/apertium-html-tools-v2/dist/.

Advantages

  • Most widely used frontend toolchain => More potential contributors, especially in GCI/GSoC
  • Most widely used frontend toolchain => Tons of online resources with growing communities
  • Sane state management => Possible to edit the code without introducing a dozen new bugs
  • Sane state management => Far fewer bugs by construction (e.g. it's no longer possible to load invalid options from local storage or select an invalid pair, also our sitemap is now valid XML...)
  • Possible to write ESNext => Considerably cleaner and more readable syntax (promises, yay!)
  • Component-driven approach => Actually testable! >40% already tested as part of setting up testing libs
  • Component-driven approach => We don't even bother rendering things that aren't on screen (making many previous hacks like data-src unnecessary and generally improving perf)
  • Component-driven approach => Much easier to introduce/remove new features, especially behind flags
  • Sane build process => >3x smaller and uses shared utilities rather than a mess of Bash + Python + Makefile
  • Sane build process => Extremely fast (<5s uncached, <1s cached), no webpack
  • Sane build process => Things we couldn't easily do before, e.g. embedding the Apertium logo directly into the page at build time
  • Smaller bundle sizes + fewer assets => Faster load time and less bandwidth
  • Lots of deleted dead code
  • Lots of minor improvements along the way (e.g. loading indicators, autofocus)

Disadvantages

  • IE11 support has been dropped. Global usage is below 1% and it's a huge crutch. If anyone with this restriction ever materializes, we can redirect them to old.apertium.org.
  • All PRs need to be ported
  • Forks need meaningfully work to port (I'm happy to help)
  • I probably introduced some minor bugs (probably less in balance than the number fixed though...)

Rollout plan

  • Current master will be moved to a v4 branch
  • This PR will land on master and tagged as v5.0.0. It will be deployed to beta.apertium.org
  • If there are no important bugs filed for beta.apertium.org, apertium.org will also be ported

Currently missing/removed features

  • Themes: this can be added ~easily if/when someone wants it (neither apertium.org nor beta.apertium.org use it)
  • Piwik integration: not sure anyone is using, can be easily added back
  • "Go to top" button: pretty sure this is broken on master
  • Browser update: don't need this anymore

If you are using/need the above features, let me know. I can probably add them back with relative ease.

cc @xavivars @unhammer @jonorthwash @TinoDidriksen

@sushain97 sushain97 force-pushed the v5-rewrite branch 2 times, most recently from 8ddcea1 to 2fb9868 Compare May 1, 2021 22:14
@jonorthwash
Copy link
Member

On mobile layouts, the localisation dropdown appears all the way down at the bottom. It would be good to to have it on the top.

@jonorthwash
Copy link
Member

Have you tested RTL localisations on mobile platforms. There are a few things that don't seem to line up nicely or flip sides. I'm not sure what was intentional and what just wasn't undertaken in your efforts.

@sushain97
Copy link
Member Author

On mobile layouts, the localisation dropdown appears all the way down at the bottom. It would be good to to have it on the top.

That's the same as the current behavior? If we want to change that, I'd rather do it as a separate issue :)

image

@sushain97
Copy link
Member Author

Have you tested RTL localisations on mobile platforms. There are a few things that don't seem to line up nicely or flip sides. I'm not sure what was intentional and what just wasn't undertaken in your efforts.

I might have missed some minor UI things -- don't think I tested RTL + mobile. I'll take a closer look.

@sushain97
Copy link
Member Author

@jonorthwash
Copy link
Member

jonorthwash commented May 2, 2021

I don't know if the issues in this screenshot with placement of language selection stuff is a new thing—I remember it being a thing in the past. But I thought I'd bring it up just in case it's new / a regression.
Screenshot from 2021-05-02 16-26-58

@jonorthwash
Copy link
Member

jonorthwash commented May 2, 2021

The menu for selecting the localisation also doesn't align language names based on their script direction, which the current html-tools does.

@sushain97
Copy link
Member Author

The menu for selecting the localisation also doesn't align language names based on their script direction, which the current html-tools does.

Ohhh, this is probably a regression, should be an easy fix. Will take a look later today.

I don't know if the issues in this screenshot with placement of language selection stuff is a new thing—I remember it being a thing in the past. But I thought I'd bring it up just in case it's new / a

What's wrong here? The wrapping on to two lines? Yeah, think that's the same.

@sushain97
Copy link
Member Author

The menu for selecting the localisation also doesn't align language names based on their script direction, which the current html-tools does.

I was not actually able to reproduce this issue in my browser but I've pushed what I think is a fix for it regardless.

@sushain97
Copy link
Member Author

Correction: reproduced and verified to be fixed using Firefox.

@sushain97 sushain97 force-pushed the v5-rewrite branch 2 times, most recently from 39daf03 to 06c1441 Compare May 5, 2021 20:02
@sushain97
Copy link
Member Author

@unhammer @xavivars I noticed y'all didn't have any 👍s here yet :) Any thoughts? Planning to merge later this week.

@unhammer
Copy link
Member

unhammer commented May 6, 2021

I haven't found anything to complain about =P It looks great 😀

@xavivars
Copy link
Member

xavivars commented May 6, 2021

Same thing, haven't seen anything, except CircleCI config missing (I guess Github Actions are the replacement).

Otherwise, looks good to me.

@sushain97 sushain97 merged commit cae0467 into master May 8, 2021
@sushain97 sushain97 deleted the v5-rewrite branch May 8, 2021 23:46
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.

URL to be translated not recognised Migrate off jQuery
4 participants