M-Labs/web2019
5
0
Fork 2
Code Issues 20 Pull Requests 3 Releases Wiki Activity

Prepare website for domain substitution #135

Merged
sb10q merged 1 commits from esavkin/web2019:134-intl-domain into master 2024-07-18 11:56:59 +08:00
Conversation 2 Commits 1 Files Changed 19 +58 -26
esavkin commented 2024-06-04 16:40:03 +08:00
Member
Copy Link

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
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
👎 1
esavkin force-pushed 134-intl-domain from 2742826bc4 to 85f30cc269 2024-06-07 11:09:40 +08:00 Compare
sb10q commented 2024-06-07 11:11:52 +08:00
Owner
Copy Link

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?
sb10q commented 2024-06-07 12:08:52 +08:00
Owner
Copy Link

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.
sb10q reviewed 2024-06-08 11:53:52 +08:00
static/js/shop/DomainedEmail.jsx Outdated
@ -0,0 +1,8 @@
import React from "react";
import {DEFAULT_DOMAIN, INTL_DOMAIN, USE_INTL_DOMAIN} from "./utils";
sb10q commented 2024-06-08 11:53:52 +08:00
Owner
Copy Link

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
static/js/shop/utils.js Outdated
@ -95,0 +100,4 @@
export const USE_INTL_DOMAIN = true;
// #!else
export const USE_INTL_DOMAIN = window.location.hostname === INTL_DOMAIN;
// #!endif
sb10q commented 2024-06-08 11:56:43 +08:00
Owner
Copy Link

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
static/js/shop_data.js Outdated
@ -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',
sb10q commented 2024-06-08 11:58:35 +08:00
Owner
Copy Link

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.
sb10q reviewed 2024-06-14 19:20:51 +08:00
static/js/shop/shop_store.js Outdated
@ -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";
sb10q commented 2024-06-14 19:20:51 +08:00
Owner
Copy Link

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

Is it really necessary to split the messages into this other file?
esavkin commented 2024-06-17 12:29:17 +08:00
Author
Member
Copy Link

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.
sb10q commented 2024-06-17 13:12:28 +08:00
Owner
Copy Link

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.
sb10q commented 2024-06-17 13:13:19 +08:00
Owner
Copy Link

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.
esavkin commented 2024-06-17 13:43:17 +08:00
Author
Member
Copy Link

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.
sb10q commented 2024-06-17 16:01:57 +08:00
Owner
Copy Link

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?
esavkin commented 2024-06-21 10:20:11 +08:00
Author
Member
Copy Link

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.
sb10q commented 2024-06-21 12:32:58 +08:00
Owner
Copy Link

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.
esavkin commented 2024-06-21 12:41:52 +08:00
Author
Member
Copy Link

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.
sb10q commented 2024-06-21 13:17:55 +08:00
Owner
Copy Link

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?
esavkin commented 2024-06-21 13:48:11 +08:00
Author
Member
Copy Link

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
templates/page-place-order.html Outdated
@ -49,0 +55,4 @@
window.DOMAIN = 'm-labs.hk';
window.API_RFQ = `https://hooks.${window.DOMAIN}/rfq`;
{% endif %}
</script>
sb10q commented 2024-06-14 19:22:10 +08:00
Owner
Copy Link

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
templates/page-place-order.html Outdated
@ -49,0 +49,4 @@
<script>
{% if "m-labs-intl.com" == get_env(name="DOMAINNAME", default="m-labs.hk") %}
window.DOMAIN = 'm-labs-intl.com';
sb10q commented 2024-06-14 19:23:28 +08:00
Owner
Copy Link

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
webpack.config.js Outdated
@ -27,2 +27,3 @@
ENV: process.env.NODE_ENV,
disable_card_highlight: false
disable_card_highlight: false,
intl_domain: false
sb10q commented 2024-06-14 19:24:02 +08:00
Owner
Copy Link

??????

??????
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
sb10q commented 2024-06-17 15:57:47 +08:00
Owner
Copy Link

Just make it a default?

Just make it a default?
esavkin commented 2024-06-17 16:19:48 +08:00
Author
Member
Copy Link

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

It's default, but updated docs for less confusion.
sb10q commented 2024-06-17 16:20:50 +08:00
Owner
Copy Link

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"
sb10q commented 2024-06-17 15:57:57 +08:00
Owner
Copy Link

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
sb10q reviewed 2024-06-21 12:33:45 +08:00
templates/shortcodes/email.html Outdated
@ -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>
sb10q commented 2024-06-21 12:33:45 +08:00
Owner
Copy Link

What is that?

What is that?
esavkin commented 2024-06-21 12:35:46 +08:00
Author
Member
Copy Link

Template to generate email link, obviously

Template to generate email link, obviously
sb10q commented 2024-06-21 12:37:48 +08:00
Owner
Copy Link

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") }}``?
esavkin commented 2024-06-21 12:38:51 +08:00
Author
Member
Copy Link

Yes

Yes
sb10q commented 2024-06-21 12:39:09 +08:00
Owner
Copy Link

Ok. This is fine then.

Ok. This is fine then.
esavkin force-pushed 134-intl-domain from 3d384007f5 to acb471e630 2024-07-08 10:50:45 +08:00 Compare
sb10q merged commit c63249e8a0 into master 2024-07-18 11:56:59 +08:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels
No items
No Label
Milestone
Clear milestone
No items
No Milestone
Assignees
Clear assignees
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.
Delete Branch "esavkin/web2019:134-intl-domain"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?