-
Notifications
You must be signed in to change notification settings - Fork 297
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
upgrade ruby version, lockfile #1816
Conversation
✅ Deploy Preview for pytorch-dot-org-preview ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
I disagree that we should delete the edge.html page without giving it more thought . If it is decided to be deprecated, it needs to be replaced with a redirect to other relevant page. Also, the whole Edge dropdown with just 1 item won't make a lot of sense. |
I chatted with @ali-khosh offline and he said he'd fine with this change if I can guarantee that the redirects are working which they are! |
@cjyabraham planning on merging this tomorrow, lmk if you have any objections? |
edge.html
Outdated
</div> | ||
|
||
|
||
redirect_to: /executorch-overview |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's weird that this thing is called .html, while it's clearly not :)
Also, I believe the dropdown would disappear as well (per preview I'm seeing). Also @svekars for context, we're "archiving" this for now since PyTorch Edge's only offering atm is ExecuTorch. We'll likely resurrect this umbrella page once we have more edge libraries. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks ok, however, you'll also need to update _includes/mobile_menu.html so that it matches your changes to main_menu.html.
I'd suggest that we not have an option in the middle of the drop down menu without a drop down. Could we instead move to under "Ecosystem" or "Learn"? |
@cjyabraham can you take a look again please?
@jennbly that's probably a better fit for a future PR, the site does need some more changes to make the flow clearer and I think I'll get more pushback removing the top level Edge drop menu since it's a nice advertisement spot. EDIT: Thought about this a bit more and you're right, especially on mobile this made the site very ugly |
The mobile menu still doesn't match with the desktop menu. BTW, why not put the "Edge" menu item under the "Learn" menu or some other menu rather than have it standing by itself? |
_includes/mobile_menu.html
Outdated
@@ -67,9 +67,6 @@ | |||
<a>Edge</a> | |||
</li> | |||
<ul class="resources-mobile-menu-items"> | |||
<li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cjyabraham not 100% sure I follow but was this not what you were asking about to make the mobile_menu match?
|
I'm having trouble running the site locally on MacOS after this upgrade. I've upgraded ruby, however, I'm now getting this: https://gist.github.com/cjyabraham/5f12926a985e77acb986bc70f4bd7125 Any advice? |
Managed to repro locally after running this
Gimme a few minutes to look into this, if it's too hard we can revert this PR |
This reverts commit fd50d96.
https://deploy-preview-1816--pytorch-dot-org-preview.netlify.app/edge
Ruby Version Upgrade:
2.7.8
to3.3.0
in.ruby-version
andGemfile
. [1] [2] - this kills most of the security warnings we have on this repoDocumentation Updates:
bash
inREADME.md
and updated thebundler
version to2.5.23
. [1] [2]EDIT: Discussed this a bit more with @cjyabraham and opted to revert the edge related doc changes