added box with brew/apt to the dl page #250
Conversation
Hahihula
requested review from
Fabricio-ESP,
alirana01,
brianignacio5 and
kolipakakondal
Summary of ChangesHello @Hahihula, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the user experience on the download page by introducing a smart, OS-aware installation guide. It aims to simplify the process for users by providing direct package manager commands (Homebrew for macOS, APT for Linux), making it easier to get started with the ESP-IDF Installation Manager without manual downloads. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds a new section to the download page to display installation instructions using brew for macOS and apt for Linux. The correct instructions are shown based on OS detection, with a fallback to show all options if the OS is unknown. This is a great improvement for user experience. My review includes a critical security fix for the apt instructions, and some suggestions to improve code maintainability by using CSS variables and avoiding inline styles.
| <pre><code># Add repository | ||
| echo "deb [trusted=yes] https://dl.espressif.com/dl/eim/apt/ stable main" | sudo tee /etc/apt/sources.list.d/espressif.list | ||
|
|
||
| sudo apt update | ||
|
|
||
| # Install CLI only | ||
| sudo apt install eim-cli | ||
|
|
||
| # Install GUI (includes CLI) | ||
| sudo apt install eim</code></pre> |
There was a problem hiding this comment.
Using [trusted=yes] in an apt sources list is a significant security vulnerability. It disables GPG signature verification for the entire repository, meaning a compromised repository or a man-in-the-middle attack could lead to malicious software being installed on the user's system. Please use the recommended secure method of adding the repository's GPG key.
<pre><code># Add Espressif's GPG key and repository
curl -fsSL https://dl.espressif.com/dl/eim/apt/espressif-archive-keyring.gpg | sudo gpg --dearmor -o /usr/share/keyrings/espressif-archive-keyring.gpg
echo "deb [signed-by=/usr/share/keyrings/espressif-archive-keyring.gpg] https://dl.espressif.com/dl/eim/apt/ stable main" | sudo tee /etc/apt/sources.list.d/espressif.list
sudo apt update
# Install CLI only
sudo apt install eim-cli
# Install GUI (includes CLI)
sudo apt install eim</code></pre>| border-bottom: 1px solid #e5e7eb; | ||
| } | ||
|
|
||
| .installation-method pre { | ||
| background: #f8fafc; | ||
| border-radius: 8px; | ||
| padding: 1rem; | ||
| overflow-x: auto; | ||
| font-family: 'Courier New', Courier, monospace; | ||
| font-size: 0.875rem; | ||
| line-height: 1.6; | ||
| border: 1px solid #e5e7eb; | ||
| } |
There was a problem hiding this comment.
For consistency and better maintainability, please use the existing CSS variables for colors instead of hardcoding them.
- On lines 113 and 124,
#e5e7ebcan be replaced withvar(--tab-border). - On line 117,
#f8fafccan be replaced withvar(--background).
| border-bottom: 1px solid #e5e7eb; | |
| } | |
| .installation-method pre { | |
| background: #f8fafc; | |
| border-radius: 8px; | |
| padding: 1rem; | |
| overflow-x: auto; | |
| font-family: 'Courier New', Courier, monospace; | |
| font-size: 0.875rem; | |
| line-height: 1.6; | |
| border: 1px solid #e5e7eb; | |
| } | |
| border-bottom: 1px solid var(--tab-border); | |
| } | |
| .installation-method pre { | |
| background: var(--background); | |
| border-radius: 8px; | |
| padding: 1rem; | |
| overflow-x: auto; | |
| font-family: 'Courier New', Courier, monospace; | |
| font-size: 0.875rem; | |
| line-height: 1.6; | |
| border: 1px solid var(--tab-border); | |
| } |
| <!-- macOS instructions --> | ||
| <div v-if="detectedOS === 'macos' || detectedOS === 'unknown'" class="installation-method"> | ||
| <h4>macOS</h4> | ||
| <div style="margin-bottom: 1rem;"> |
There was a problem hiding this comment.
Please avoid using inline styles like style="margin-bottom: 1rem;". It's a best practice to define styles in a CSS file using classes for better maintainability and separation of concerns. You could create a new class for this and apply it here and on the similar div on line 551.
Just adding the information about brew and apt to the dl page so users can easily follow.
it has os detection feature so it shows bre to macos users and apt to linux user and at the moment nothing to the windows user ... if the detection fails it shows all the variants....
