Skip to content
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

refactor(help): update mqtt page header links #1254

Merged
merged 1 commit into from
Mar 31, 2023
Merged

refactor(help): update mqtt page header links #1254

merged 1 commit into from
Mar 31, 2023

Conversation

ysfscream
Copy link
Member

PR Checklist

If you have any questions, you can refer to the Contributing Guide

What is the current behavior?

Please describe the current behavior and link to a relevant issue.

Issue Number

Example: #123

What is the new behavior?

Please describe the new behavior or provide screenshots.

image

Does this PR introduce a breaking change?

  • Yes
  • No

Specific Instructions

Are there any specific instructions or things that should be known prior to review?

Other information

@ysfscream ysfscream added enhancement New feature or request web MQTTX Web desktop MQTTX Desktop labels Mar 31, 2023
@ysfscream ysfscream self-assigned this Mar 31, 2023
ja: 'MQTT 5 を探る',
tr: 'MQTT 5 Keşfedin',
hu: 'Fedezze fel az MQTT 5-öt',
},
guideTitle: {
zh: 'MQTT 入门',
en: 'MQTT Beginner’s Guide',

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the code review:

  1. The code looks well formatted and it is easy to read.
  2. It looks like the code is being used to add language support for a public MQTT broker and MQTT 5 Explore.
  3. The code is adding new keys and values to the existing object, which is a valid way of adding language support.
  4. The code is using proper indentation and spacing which is good.
  5. The code does not contain any syntax errors and should run without any issues.

@@ -32,6 +32,8 @@ export default (lang: Language) => {
docs: `${MQTTXSite}/docs${utm}mqttx-help-to-docs`,
forum: `${forumSite}${utm}mqttx-help-to-forum`,
learnMQTT: `${EMQSite}/mqtt${utm}mqttx-help-to-learn-mqtt`,
publicMqttBroker: `${EMQSite}/mqtt/public-mqtt5-broker`,
mqtt5: `${EMQSite}/mqtt/mqtt5`,
blogUtm: `${utm}mqttx-help-to-blog`,
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the code review:

  1. The code patch looks correct. It adds two additional lines to the existing code, which link to EMQ's website for public MQTT brokers and MQTT5.
  2. The code is well indented and follows a consistent style.
  3. The new lines of code are consistent with the existing code, and the variable names are descriptive and easy to understand.
  4. All the links are properly formatted and appear to be valid.
  5. There are no unnecessary variables or functions.
  6. There are no potential security risks associated with the code.

link: learnMQTT,
icon: 'icon-mqtt5',
title: this.$tc('help.mqtt5Explore'),
link: mqtt5,
},
]
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

our code review by looking at the changes made on line 62 and 63.

In the original code, the "helpTop" method was retrieving the "docs" and "forum" links from the gaCustomLinks() method. In the updated code, the "learnMQTT", "publicMqttBroker" and "mqtt5" links are being retrieved instead. This change looks to be valid as it is more reflective of the current state of the application.

Next, we can look at the changes made on lines 64-69. In the original code, the "helpTop" method was setting the "icon", "title" and "link" keys in the array for the "docs" and "forum" links. However, in the updated code, these keys have been updated to match the new links being retrieved in the "helpTop" method, namely "learnMQTT", "publicMqttBroker" and "mqtt5". This change looks to be valid as it is necessary in order to reflect the new links being retrieved.

Finally, we can look at the changes made on lines 70-75. In the original code, the "helpTop" method was setting the "icon", "title" and "link" keys in the array for the "learnMQTT" link. However, in the updated code, these keys have been updated to match the new link being retrieved in the "helpTop" method, namely "mqtt5". This change looks to be valid as it is necessary in order to reflect the new link being retrieved.

Overall, the changes made to the code look to be valid and necessary in order to reflect the new links being retrieved in the "helpTop" method.

ja: 'MQTT 5 を探る',
tr: 'MQTT 5 Keşfedin',
hu: 'Fedezze fel az MQTT 5-öt',
},
guideTitle: {
zh: 'MQTT 入门',
en: 'MQTT Beginner’s Guide',

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the code review.

  1. The patch adds additional translations for the strings 'publicMqttBroker', 'mqtt5Explore' and 'guideTitle' for Chinese, English, Japanese, Turkish and Hungarian languages. This looks good and the syntax used is correct.

  2. The patch also adds new lines at the end of the file. This is not necessary and should be removed.

  3. It is also recommended to add comments to explain why these changes are being made. This helps other developers understand the code better.

Overall, the patch looks good.

@@ -17,6 +17,8 @@ export default (lang: Language) => {
docs: `${MQTTXSite}/docs${utm}mqttx-web-help-to-docs`,
forum: `${forumSite}${utm}mqttx-web-help-to-forum`,
learnMQTT: `${EMQSite}/mqtt${utm}mqttx-web-help-to-learn-mqtt`,
publicMqttBroker: `${EMQSite}/mqtt/public-mqtt5-broker`,
mqtt5: `${EMQSite}/mqtt/mqtt5`,
blogUtm: `${utm}mqttx-web-help-to-blog`,
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with the code review.

The code patch adds two new lines of code to the existing code. The first line adds a link to a public MQTT broker. The second line adds a link to an MQTT5 page.

The added lines of code appear to be well-formatted and do not introduce any syntax errors. Furthermore, they add useful information that can help users access additional resources.

Overall, this code patch looks good and should be accepted.

link: learnMQTT,
icon: 'icon-mqtt5',
title: this.$tc('help.mqtt5Explore'),
link: mqtt5,
},
]
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the code review:

  1. The original code uses "docs" and "forum" as two properties of the gaCustomLinks method, while the patch replaces them with "learnMQTT", "publicMqttBroker" and "mqtt5". This is a good change because it provides more options for users to learn about MQTT.

  2. The original code uses "icon-docs" and "icon-forum" as icons for the links, while the patch replaces them with "icon-learn-mqtt", "icon-public-mqtt-broker" and "icon-mqtt5". This is a good change because it makes the icons more relevant to the type of link they are linking to.

  3. The original code uses "this.$tc('help.docs')" and "this.$tc('help.forum')" as titles for the links, while the patch replaces them with "this.$tc('help.learn')", "this.$tc('help.publicMqttBroker')" and "this.$tc('help.mqtt5Explore')". This is a good change because it gives more descriptive titles for the links.

Overall, this code patch looks good and should be approved.

@Red-Asuka Red-Asuka merged commit c43035b into main Mar 31, 2023
@ysfscream ysfscream deleted the dev/packages branch March 31, 2023 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
desktop MQTTX Desktop enhancement New feature or request web MQTTX Web
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants