1433499 - [FTL] Refactor fluent_interface.js a bit

Status: RESOLVED FIXED

(Webtools Graveyard :: Pontoon, enhancement, P2)

Description

[Axel Hecht [:Pike]]

After glancing at two patches for the fluent UI in pontoon, I've got some high-level comments.

Can we (and by that I mean probably mathjazz, let's be honest) refactor the js code so that nomenclature in the code is closer to the AST?

My main problem is that there's a lot of somethingElement or Elements, and I for the sake of god can't map that to AST fragments in fluent, which makes it really hard to figure out if that code handles fluent well.

And if we do so, are there js parts that we could test w/out needing the DOM? Now that we have tests for js ;-).

Comment 1

[[DEACTIVATED] Adrian Gaudebert]

I would argue that we should keep that work for when we rewrite all of that code for Translate.Next.

Comment 2

[Axel Hecht [:Pike]]

I did contemplate waiting for Translate.Next.

My concern is that we'll need to do a ton of improvements on the fluent UI now, and the overlap between the people understanding fluent and fluent.js and the folks that understand pontoon front end is ... Matjaz?

My trigger for filing this bug was your question in https://github.com/mozilla/pontoon/pull/814#issuecomment-360729248.

And quite frankly, I couldn't say yes. All I know is that there wasn't a singled-out line in the js or python code that made me think that I could propose a counter example. That's far from a "yes, this works as designed".

Comment 3

[Matjaz Horvat [:mathjazz]]

:adrian, I agree that it makes more sense to do a proper rewrite as part of Translate.Next. At that stage we should also have a much better idea of what the Fluent specific UI/UX should be.

But the title says "a bit" and I'd like to take that seriously :) I was actually planning to make some code readability improvements as part of bug 1431436, but decided to push it back to avoid blocking 1424682.

I was mostly thinking about:

- Naming: I agree about using the language that more closely matches AST. It's not always possible (because placeables are a different thing in Pontoon and FTL), but most of the time it is. We even have internal inconsistencies about some terms (e.g. "element"). And we also have a lot of serializeX functions.

- Code structure: Some functions are public (Pontoon.fluent.X()), but don't need to be. Order of functions in the file is also pretty random.

- Comments: Some could be more verbose.

I don't think I can commit to anything much bigger than this in Q1.

Comment 4

[GitHub Bugzilla PR Linker]

Attachment: Link to GitHub pull-request: https://github.com/mozilla/pontoon/pull/911

Comment 5

[[github robot]]

Commit pushed to master at https://github.com/mozilla/pontoon

https://github.com/mozilla/pontoon/commit/395533daee05dde8757a46c3e392b3ce1eb5a78f
Fix bug 1433499: Refactor fluent_interface.js (#911)

* Drop isSelectExpressionElement() and use AST enquiry instead
* Drop focusFirstField(), not need to be factored out
* Make runChecks() an internal function
* Make toggleEditorToolbar() an internal function
* Remove unused `Add attribute` code
* Factor out FTL editor event handlers
* Use relative reference
* Make isSimpleElement(), isPluralElement(), isSupportedMessage(),
  isSimpleMessage() internal functions
  and rename isSimpleString()to isSimpleMessage()
* Make serializePlaceables() internal function and rename it to
  stringifyElements()
* Make getTextareaElement() internal function and rename it to
  renderTextareaElement()
* Fix comments and param names of internal functions
* Rename serializeElements() to serializeFTLEditorElements() and rename
  and document function parameters
* Reorder public Pontoon.fluent.X() functions, fix comments and rename:
  - getTranslationSource() to getFTLEditorContentsAsSource()
  - toggleOriginal() to renderOriginal()
* Make elementsSupported() an internal function
  and rename it to areSupportedElements()
* Simplify isSimpleMessage()
* Fix textarea detection for placeable insertion. Regression from:
  https://github.com/mozilla/pontoon/pull/840/commits/70b85239b4836f469b8f7548a2078b8960dab254