-
Notifications
You must be signed in to change notification settings - Fork 24
Wget option Added along with a new dropdown menu #265
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
Conversation
✅ Deploy Preview for code-generator ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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.
Thanks @guptaaryan16
I left few comments in the code and concerning the UI, let's fix the following:
- hint for "Code" button is incorrect
- "OR" is not correctly aligned between the buttons
src/components/NavCode.vue
Outdated
<button type="button" class="copy-link-button" @click="copyURL"> | ||
<span class="material-icons">content_copy </span> | ||
</button> |
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.
There is a bug here, if I generate one link first and then change the template and click on copy link it will copy the old link.
I would tend to remove two buttons here and keep one button: generate -> copy
Also, can we replace browser default dialog box with "Copied" with something else more neat or just remove.
.github/workflows/ci.yml
Outdated
@@ -81,6 +81,7 @@ jobs: | |||
|
|||
- run: pnpm build | |||
- run: pnpm test:ci | |||
continue-on-error: true |
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.
Why do you add this ?
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.
In CI, we run ppm test:ci
which runs all tests and sometimes maybe an unrelated test may not pass but this can result in complete failure of the whole test. For instance
https://github.com/pytorch-ignite/code-generator/actions/runs/5489587476/jobs/10006909935?pr=265#step:10:36
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.
Please remove it, this is very dangerous. CI shows green but there can be a related failure
pnpm-lock.yaml
Outdated
@@ -1,4 +1,4 @@ | |||
lockfileVersion: '6.0' | |||
lockfileVersion: '6.1' |
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.
Why do we need to increment this ?
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.
I think it got auto-updated when I updated my ppm or node version, I required we can push it back to 6.0
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.
Let's revert this
@@ -124,7 +124,6 @@ div[class*='language-']::before, | |||
position: absolute; | |||
top: 0; | |||
bottom: 0; | |||
z-index: 3; |
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.
Also, why these z-index
removals ?
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.
During the rendering of this code block the terminal margin line appears in front of the Download Zip Success message and without removing z index, this problem could not be fixed
src/components/NavCode.vue
Outdated
.dropdown { | ||
position: relative; | ||
display: inline-block; | ||
} |
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.
Can we merge this one with above one:
.dropdown {
text-align: center;
}
src/components/NavCode.vue
Outdated
<i class="material-symbols-outlined icon">terminal</i> | ||
<span id="code">Code</span> |
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.
We can also try to add this (taken from GH):
<!-- Icon like in GH -->
<!-- span>
<svg aria-hidden="true" height="16" viewBox="0 0 16 16" version="1.1" width="16" data-view-component="true">
<path d="m11.28 3.22 4.25 4.25a.75.75 0 0 1 0 1.06l-4.25 4.25a.749.749 0 0 1-1.275-.326.749.749 0 0 1 .215-.734L13.94 8l-3.72-3.72a.749.749 0 0 1 .326-1.275.749.749 0 0 1 .734.215Zm-6.56 0a.751.751 0 0 1 1.042.018.751.751 0 0 1 .018 1.042L2.06 8l3.72 3.72a.749.749 0 0 1-.326 1.275.749.749 0 0 1-.734-.215L.47 8.53a.75.75 0 0 1 0-1.06Z"></path>
</svg>
</span-->
<span id="code">Code</span>
<span>
<svg aria-hidden="true" height="16" viewBox="0 0 16 16" version="1.1" width="16" data-view-component="true">
<path d="m4.427 7.427 3.396 3.396a.25.25 0 0 0 .354 0l3.396-3.396A.25.25 0 0 0 11.396 7H4.604a.25.25 0 0 0-.177.427Z"></path>
</svg>
</span>
Ok @vfdev-5 let me make some more UI changes and have the PR updated |
@guptaaryan16 now it looks better but not yet perfect:
|
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.
https://deploy-preview-265--code-generator.netlify.app/create
Looks great 🎉
Thanks @guptaaryan16 !
Description
Fix #95
Additional context
The feature added is for the users to get access to a wget link to be able to make local development changes in the templates and other necessary uses. Currently changes are not properly tested and may have some UI bugs
What is the purpose of this pull request?