Prepare website for domain substitution #135

Open
esavkin wants to merge 5 commits from esavkin/web2019:134-intl-domain into master
Member

This PR prepares the website and shop for #134:

  1. All emails with m-labs.hk will be substituted with m-labs-intl.com when served with respective config option
  2. Some links (not to the documentation though)

Substitution happens in JS runtime in following events:

  1. Webshop: before rendering email

Also added preprocessor parameters for testing locally.

This way the mirror can directly pull the HTML or this repo and serve it.

This PR prepares the website and shop for #134: 1. All emails with m-labs.hk will be substituted with m-labs-intl.com when served with respective config option 2. Some links (not to the documentation though) Substitution happens in JS runtime in following events: 1. Webshop: before rendering email Also added preprocessor parameters for testing locally. This way the mirror can directly pull the HTML or this repo and serve it.
esavkin added 1 commit 2024-06-04 16:40:04 +08:00
2742826bc4 Prepare website for domain substitution
Signed-off-by: Egor Savkin <es@m-labs.hk>
esavkin force-pushed 134-intl-domain from 2742826bc4 to 85f30cc269 2024-06-07 11:09:40 +08:00 Compare
Owner

Client-side in JS, seriously? Can't you just set an environment variable to be read by Zola?

Client-side in JS, seriously? Can't you just set an environment variable to be read by Zola?
esavkin added 1 commit 2024-06-07 11:49:14 +08:00
b507cf5d79 Use template replacer instead of JS (except webshop)
Signed-off-by: Egor Savkin <es@m-labs.hk>
Owner

Can you do it without "replacement"? I can't believe the code is still such a mess.

Can you do it without "replacement"? I can't believe the code is still such a mess.
esavkin added 1 commit 2024-06-07 17:44:36 +08:00
7c06c58e62 Use shortcodes for email links
Signed-off-by: Egor Savkin <es@m-labs.hk>
sb10q reviewed 2024-06-08 11:53:52 +08:00
@ -0,0 +1,8 @@
import React from "react";
import {DEFAULT_DOMAIN, INTL_DOMAIN, USE_INTL_DOMAIN} from "./utils";
Owner

We need three variables instead of one because?

We need three variables instead of one because?
sb10q reviewed 2024-06-08 11:56:44 +08:00
@ -95,0 +100,4 @@
export const USE_INTL_DOMAIN = true;
// #!else
export const USE_INTL_DOMAIN = window.location.hostname === INTL_DOMAIN;
// #!endif
Owner

Nix/Hydra can simply create one .js file that defines DOMAIN = "xxxx" and replace all this nonsense.
So many problems created by your laziness and resistance to advice.

Nix/Hydra can simply create one .js file that defines ``DOMAIN = "xxxx"`` and replace all this nonsense. So many problems created by your laziness and resistance to advice.
sb10q reviewed 2024-06-08 11:58:35 +08:00
@ -1,6 +1,7 @@
const shop_data = {
API_RFQ: 'https://hooks.m-labs.hk/rfq',
INTL_API_RFQ: 'https://m-labs-intl.com/api/rfq',
Owner

Just define SHOP_API_RFQ in a generated config.js together with DOMAIN.

Just define SHOP_API_RFQ in a generated config.js together with DOMAIN.
esavkin added 1 commit 2024-06-11 17:33:11 +08:00
33c7936e72 Inject JS in templates with correct domains
Signed-off-by: Egor Savkin <es@m-labs.hk>
sb10q reviewed 2024-06-14 19:20:51 +08:00
@ -10,6 +10,7 @@ import {TriggerCrateWarnings, TriggerWarnings} from "./warnings";
import {Validation, validateEmail, validateNote, validateJSONInput} from "./validate";
import {CratesToJSON, JSONToCrates} from "./json_porter";
import {ProcessOptionsToData} from "./options/Options";
import {RFQMessages} from "./RFQMessages";
Owner

Is it really necessary to split the messages into this other file?

Is it really necessary to split the messages into this other file?
Author
Member

Well, the loader is the same for both JS and JSX, and it works fine if you move RFQMessages object into shop_store and import React, but pure JS files do not permit JSX (unless you import react) and it would be quite unexpected for developers to see JSX code in JS file.

Also, the shop_store.js is already quite big and better be split in future.

So both reasons are styling and best practices thing.

Well, the loader is the same for both JS and JSX, and it works fine if you move RFQMessages object into shop_store and import React, but pure JS files do not permit JSX (unless you import react) and it would be quite unexpected for developers to see JSX code in JS file. Also, the shop_store.js is already quite big and better be split in future. So both reasons are styling and best practices thing.
Owner

A few-lines file with a couple strings is not good style and not best practice.

A few-lines file with a couple strings is not good style and not best practice.
Owner

And it does not help with the size of shop_store.js either.

And it does not help with the size of shop_store.js either.
Author
Member

It's not a couple of strings - it's a JSX code, one of the fields of which contains an actual link mailto:. If it was just a string-only solution, it would appear in the page as <a href="....">email</a>, or without mailto links, which makes it less convenient.

Also since we substitute domains, it is better to use some unified method of doing it, i.e. utilize DomainedEmail component, that's why I added a link in this message.

I'll merge RFQMessages into DomainedEmail, though it is not exactly good for component hierarchy.

It's not a couple of strings - it's a JSX code, one of the fields of which contains an actual link `mailto:`. If it was just a string-only solution, it would appear in the page as `<a href="....">email</a>`, or without mailto links, which makes it less convenient. Also since we substitute domains, it is better to use some unified method of doing it, i.e. utilize `DomainedEmail` component, that's why I added a link in this message. I'll merge `RFQMessages` into `DomainedEmail`, though it is not exactly good for component hierarchy.
Owner

Cannot just access window.DOMAIN here, as you do in JS in other places?

Cannot just access window.DOMAIN here, as you do in JS in other places?
Author
Member

Of course it can, just I don't want to populate external variables in the code.

Of course it can, just I don't want to populate external variables in the code.
Owner

Then I don't see what the problem is. Splitting some random "RFQ" strings into Domained.jsx does not make any sense to me.

Then I don't see what the problem is. Splitting some random "RFQ" strings into Domained.jsx does not make any sense to me.
Author
Member

I'm now not sure what do you want from this. If you want it to remain in shop_store, then I explained earlier why it is not suitable here.

I'm now not sure what do you want from this. If you want it to remain in shop_store, then I explained earlier why it is not suitable here.
Owner

What's the issue if you access window.DOMAIN in shop_store.js and keep the strings there?

What's the issue if you access window.DOMAIN in shop_store.js and keep the strings there?
Author
Member

For what purpose?
With this PR the email in RFQ error case becomes clickable, which is an usability improvement.
These "strings" are JSX elements now, i.e. rendered as HTML, and not as a text. If they would remain strings, the clickable email wouldn't be possible, because it would be rendered as text (i.e. user would see the html itself).
Since normally JS doesn't contain JSX elements, it would be a weird practice to make shop_store.js depend on React to add such elements. Also, I introduced shop_store.js in earlier refactoring to keep data/logic away from rendering as much as possible, which is a standard practice. So putting the "strings" (actually elements) back to shop_store would be a regress.
Also spreading dependencies on external variables across code is not a good practice either. I admit it is not in ideal state right now, but it was far worse before.

Of course it is possible to make something like template string and leave the email unclickable, but it is a regress relative to this PR in usability and in code quality relative to before this PR.

For what purpose? With this PR the email in RFQ error case becomes clickable, which is an usability improvement. These "strings" are JSX elements now, i.e. rendered as HTML, and not as a text. If they would remain strings, the clickable email wouldn't be possible, because it would be rendered as text (i.e. user would see the html itself). Since normally JS doesn't contain JSX elements, it would be a weird practice to make shop_store.js depend on React to add such elements. Also, I introduced shop_store.js in earlier refactoring to keep data/logic away from rendering as much as possible, which is a standard practice. So putting the "strings" (actually elements) back to shop_store would be a regress. Also spreading dependencies on external variables across code is not a good practice either. I admit it is not in ideal state right now, but it was far worse before. Of course it is possible to make something like template string and leave the email unclickable, but it is a regress relative to this PR in usability and in code quality relative to before this PR.
sb10q reviewed 2024-06-14 19:22:10 +08:00
@ -49,0 +55,4 @@
window.DOMAIN = 'm-labs.hk';
window.API_RFQ = `https://hooks.${window.DOMAIN}/rfq`;
{% endif %}
</script>
Owner

Just define a hooks.m-labs-intl.com instead of this nonsense.

Just define a hooks.m-labs-intl.com instead of this nonsense.
sb10q reviewed 2024-06-14 19:23:28 +08:00
@ -49,0 +49,4 @@
<script>
{% if "m-labs-intl.com" == get_env(name="DOMAINNAME", default="m-labs.hk") %}
window.DOMAIN = 'm-labs-intl.com';
Owner

window.DOMAIN = get_env(name="DOMAINNAME", default="m-labs.hk") ?

``window.DOMAIN = get_env(name="DOMAINNAME", default="m-labs.hk")`` ?
sb10q reviewed 2024-06-14 19:24:02 +08:00
@ -27,2 +27,3 @@
ENV: process.env.NODE_ENV,
disable_card_highlight: false
disable_card_highlight: false,
intl_domain: false
Owner

??????

??????
esavkin marked this conversation as resolved
esavkin force-pushed 134-intl-domain from 33c7936e72 to e06ce29220 2024-06-17 15:57:02 +08:00 Compare
sb10q reviewed 2024-06-17 15:57:47 +08:00
README.md Outdated
@ -16,3 +16,3 @@
```
zola serve
DOMAINNAME=m-labs.hk zola serve
Owner

Just make it a default?

Just make it a default?
Author
Member

It's default, but updated docs for less confusion.

It's default, but updated docs for less confusion.
Owner

Just say you can set the DOMAINNAME environment variable to something else. It doesn't need to feature prominently.

Just say you can set the DOMAINNAME environment variable to something else. It doesn't need to feature prominently.
sb10q reviewed 2024-06-17 15:57:57 +08:00
config.toml Outdated
@ -11,3 +11,4 @@ highlight_code = true
[extra]
author = "M-Labs"
domain = "m-labs.hk"
Owner

What is this for?

What is this for?
esavkin marked this conversation as resolved
esavkin force-pushed 134-intl-domain from e06ce29220 to 1ff79878be 2024-06-17 16:17:44 +08:00 Compare
esavkin added 1 commit 2024-06-21 10:36:34 +08:00
3d384007f5 Update readme
Signed-off-by: Egor Savkin <es@m-labs.hk>
sb10q reviewed 2024-06-21 12:33:45 +08:00
@ -0,0 +1 @@
<a href="mailto:{{address}}@{{get_env(name="DOMAINNAME", default="m-labs.hk")}}">{{address}}@{{get_env(name="DOMAINNAME", default="m-labs.hk")}}</a>
Owner

What is that?

What is that?
Author
Member

Template to generate email link, obviously

Template to generate email link, obviously
Owner

Considering your constant rate of mistakes I better double-check instead of assuming it's "obvious". So this is what gets called when you use {{ email(address="sb") }}?

Considering your constant rate of mistakes I better double-check instead of assuming it's "obvious". So this is what gets called when you use ``{{ email(address="sb") }}``?
Author
Member

Yes

Yes
Owner

Ok. This is fine then.

Ok. This is fine then.
This pull request can be merged automatically.
You are not authorized to merge this pull request.
You can also view command line instructions.

Step 1:

From your project repository, check out a new branch and test the changes.
git checkout -b esavkin-134-intl-domain master
git pull 134-intl-domain

Step 2:

Merge the changes and update on Gitea.
git checkout master
git merge --no-ff esavkin-134-intl-domain
git push origin master
Sign in to join this conversation.
No reviewers
No Label
No Milestone
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: M-Labs/web2019#135
No description provided.