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

Wget option Added along with a new dropdown menu #265

Merged
merged 30 commits into from
Jul 14, 2023

Conversation

guptaaryan16
Copy link
Contributor

@guptaaryan16 guptaaryan16 commented Jun 26, 2023

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?

  • Bug fix
  • New feature
  • Other

@netlify
Copy link

netlify bot commented Jun 26, 2023

Deploy Preview for code-generator ready!

Name Link
🔨 Latest commit 76cfe0a
🔍 Latest deploy log https://app.netlify.com/sites/code-generator/deploys/64b1151b1e654e0008d5caf1
😎 Deploy Preview https://deploy-preview-265--code-generator.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@guptaaryan16 guptaaryan16 marked this pull request as draft June 26, 2023 14:11
@guptaaryan16 guptaaryan16 marked this pull request as ready for review July 4, 2023 12:49
@vfdev-5 vfdev-5 changed the title Wget option Added along with a new dropdown menu [WIP] Wget option Added along with a new dropdown menu Jul 4, 2023
Copy link
Member

@vfdev-5 vfdev-5 left a 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:

  1. hint for "Code" button is incorrect
  2. "OR" is not correctly aligned between the buttons

image

functions/colab.js Show resolved Hide resolved
src/components/NavCode.vue Outdated Show resolved Hide resolved
Comment on lines 29 to 31
<button type="button" class="copy-link-button" @click="copyURL">
<span class="material-icons">content_copy </span>
</button>
Copy link
Member

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.

pnpm-lock.yaml Outdated Show resolved Hide resolved
functions/code.js Outdated Show resolved Hide resolved
@@ -81,6 +81,7 @@ jobs:

- run: pnpm build
- run: pnpm test:ci
continue-on-error: true
Copy link
Member

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 ?

Copy link
Contributor Author

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

Copy link
Member

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'
Copy link
Member

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 ?

Copy link
Contributor Author

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

Copy link
Member

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;
Copy link
Member

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 ?

Copy link
Contributor Author

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

Comment on lines 197 to 200
.dropdown {
position: relative;
display: inline-block;
}
Copy link
Member

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;
}

Comment on lines 10 to 11
<i class="material-symbols-outlined icon">terminal</i>
<span id="code">Code</span>
Copy link
Member

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>

@vfdev-5
Copy link
Member

vfdev-5 commented Jul 10, 2023

If we could make this drop down menu like in GH, it would be very nice:
image

I mean the lines separating the options and text edit + copy button

@guptaaryan16
Copy link
Contributor Author

Ok @vfdev-5 let me make some more UI changes and have the PR updated

@vfdev-5
Copy link
Member

vfdev-5 commented Jul 10, 2023

@guptaaryan16 now it looks better but not yet perfect:
image

<> Code is not aligned, but when I tried it was rather complicated to align ...
and text edit widget still on the left instead of center...
Maybe we can remove the icon and "Clone" ?

Copy link
Member

@vfdev-5 vfdev-5 left a comment

Choose a reason for hiding this comment

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

@guptaaryan16 guptaaryan16 merged commit 8286377 into pytorch-ignite:main Jul 14, 2023
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use wget with download link
2 participants