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

Update Link Container #49

Merged
merged 9 commits into from
May 26, 2024
Merged

Update Link Container #49

merged 9 commits into from
May 26, 2024

Conversation

kimchiiboiii
Copy link
Contributor

Title

Update Link Container

Description

Added event listeners to inputs in the link class, added a prototype isValidURL function that uses a regex to check
if a string is a valid URL. Added function setIcon that fetches the corresponding icon or the default icon.

Added default.png for the icon class if there is no link in socialLinks or if ${key.png} exists.

Implemented previewLink(button), matching inputValue with a key in socialLinks if possible and handling the username.


Issue Ticket Number

Fixes #42


Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Checklist:

  • I have followed the contributing guidelines of this project as mentioned in CONTRIBUTING
  • I have checked to ensure there aren't other open Pull Requests for the same update/change?
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes needed to the documentation

Copy link
Owner

@Param302 Param302 left a comment

Choose a reason for hiding this comment

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

Hey @kimchiiboiii ,

Thanks for your contribution, there are some bugs in the code, please fix it and I will merge your PR.

I have added the comments for the bugs, please check once.

@@ -112,6 +123,49 @@ function previewLink(button) {
console.log("Placeholder value:", inputElement.placeholder);
console.log("Input value:", inputValue);
// Rest of the code...
if (isValidURL(inputValue)) {
Copy link
Owner

Choose a reason for hiding this comment

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

You can bypass this as of now...

Comment on lines 130 to 140
const matchingKey = Object.keys(socialLinks).find(key => {
const inputValueNoUsername = socialLinks[key].replace('<username>', '');
return inputValue.startsWith(inputValueNoUsername) && inputValue.length > inputValueNoUsername.length;
// ^ This ensures the link has text in the <username> placeholder before returning
});
if(matchingKey) {
console.log("Key found!:", matchingKey);
} else {
console.log("No key found!");
}
setIcon(matchingKey, parentElement);
Copy link
Owner

Choose a reason for hiding this comment

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

It's not working, I have tried manually too, not matching with the name of the social handle.
Check screenshots attached.
Screenshot from 2024-05-17 21-54-33

Its showing the default icon only.
When https://linkedin minimum this keyword should be entered, it should show linkedin icon.

I have bypassed the ValidURL if statement with this:

    if (inputValue.includes("https://linkedin.com")) {
        console.log("Valid URL!")
        button.disabled = false;
        
        const matchingKey = Object.keys(socialLinks).find(key => {
            const inputValueNoUsername = socialLinks[key].replace('<username>', '');
            return inputValue.startsWith(inputValueNoUsername) && inputValue.length > inputValueNoUsername.length;
            // ^ This ensures the link has text in the <username> placeholder before returning
        });
		...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When you commented above "You can bypass this as of now." you mean don't worry about it anymore, correct?
Because I noticed you added link validation so I deleted mine. Just making sure that I understood correctly.

Ahh I see, I didn't notice that some of the key values started with "www," and others didn't.

    let matchingKey;
    const matchingLink = Object.entries(socialLinks).find(([key, value]) => {
        const url = new URL(value.replace("<username>", ""));
        const domainKeyWords = url.hostname.replace("www.", "").replace(".com", "");
        const isMatch = inputValue.includes(domainKeyWords);

So instead I'll get the value of the matching socialLink key, take it's domain name and remove www. and .com,
so that as soon as https://linkedin is entered it will retrieve the key and show the icon.
But right now it will still retrieve the key and show the icon if you were to just enter linkedin because
I removed isValidURL.

Let me know if anything needs work, thanks.

Copy link
Owner

@Param302 Param302 left a comment

Choose a reason for hiding this comment

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

Hey @kimchiiboiii ,
There's a conflict is coming of default.png.
You can fetch the latest code, it has default.png file and use that one.
Also, fetch and update with the latest code, rest your code is fine, do the required changes I will merge it.

@kimchiiboiii kimchiiboiii reopened this May 20, 2024
let matchingKey;
const matchingLink = Object.entries(socialLinks).find(([key, value]) => {
const url = new URL(value.replace("<username>", ""));
const domainKeyWords = url.hostname.replace("www.", "").replace(".com", "");
Copy link
Owner

Choose a reason for hiding this comment

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

Hey,
See all URLs doesn't have www, please go through the social-links.json, and see the format of URLs, it should be generalized for all types of URLs.
For few it is working, but not for all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey,
I added in handling for the socialLinks values that start with the same domain keywords such as:
"dev.to", "devpost.com", "devfolio.co" etc.

I was stuck on how retrieve the key as soon as the minimum keyword is entered like https://www.linkedin while also not conflicting with the "dev" URLs but I think this code should handle it and any URLs in the future that start with the same domain keywords.

@Param302
Copy link
Owner

Hey @kimchiiboiii ,

Thanks for your contribution, the code is working but there is still some bug 😅 , but no worries, I will handle it.
As you've done so much effort, really hats off 🫡
I am currently refactoring the code, once it's done, I will merge your PR.

@Param302 Param302 merged commit ca6a651 into Param302:main May 26, 2024
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.

[FEATURE] Update Link Container
2 participants