alexeyinkin commented on code in PR #22960:
URL: https://github.com/apache/beam/pull/22960#discussion_r958532408


##########
website/www/site/assets/js/language-switch-v2.js:
##########
@@ -26,144 +26,240 @@ $(document).ready(function() {
         $('html, body').scrollTop(elCurrentHeight - 
elPreviousOffsetFromViewPort);
     }
 

Review Comment:
   Integrating these messages could be done with less changes, but I took the 
chance to refactor this file.
   The mechanism here had a lot of problems:
   
   - Unclear use of terms: `id`, `name`, `prefix`, `wrapper`. Changed all of 
that to only one: `name`.
   - Unclear use of terms: `lang`, `langs` (even to mean a single string), 
`pref`, `type`, `value`. Changed to only `value` / `values`.
   - Inconsistent use of `default` term: it was `java` at the invocation and 
`language-java` later. Changed to a short value without a prefix.
   - Inconsistent use of strings like `java` and `language-java`, redundant 
conversion between them, redundant storage of `language-xxx` as the value for 
`language` setting.
   - `langs` was used for values even when rendering switches other than 
`language`.
   - Special casing for turning 'py' to 'python' in a ternary was not scalable 
if new special cases appear. Changed to a scalable `valueToTabTitle()` method. 
We can also easily add the coming use of `TypeScript` which otherwise would 
have been capitalized as `Typescript`.
   
   So all of that justifies the big change.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to