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

Display name should be filled in with the domain name, without the TLD #992

Open
gorkem-bwl opened this issue Oct 20, 2024 · 25 comments
Open
Labels
good first issue Good for newcomers

Comments

@gorkem-bwl
Copy link
Contributor

When adding a new monitor, and user adds "URL to monitor", the Display name should be filled in with the URL, without the TLD. The first letter should be capitalized.

Examples:

https://3dicons.co/ -> 3dicons
https://allaboutberlin.com -> Allaboutberlin

@gorkem-bwl gorkem-bwl added the good first issue Good for newcomers label Oct 20, 2024
@Khushal-Pabri
Copy link

I would like to work on this issue. Could you please assign it to me?

@ajhollid
Copy link
Collaborator

@Khushal-Pabri sure, go for it.

Please make a well formatted and descriptive PR when you're ready to go.

Thanks for your interest in the project!

@gorkem-bwl
Copy link
Contributor Author

gorkem-bwl commented Oct 24, 2024

@Khushal-Pabri checking here. Are you still interested in sending a PR for this issue?

@gorkem-bwl gorkem-bwl assigned shyna92 and unassigned Khushal-Pabri Nov 8, 2024
@gorkem-bwl
Copy link
Contributor Author

@shyna92 assigned to you, fyi.

@peterpardo
Copy link

@gorkem-bwl Hello! I can work on this. Can you assign me this issue?

@gorkem-bwl
Copy link
Contributor Author

Yes, please go ahead. Make sure you run the app and test it live, and then send a video showing how it works.

@peterpardo
Copy link

@gorkem-bwl Hello! Just a question regarding this issue:

  1. If the user inputs a URL, will we simultaneously fill up the Display Name field?
  2. After they type in the URL, and they changed the Display Name, and then they change again the URL, how will the Display Name behave?

@gorkem-bwl
Copy link
Contributor Author

  1. Yes, I assume it's better to do this way. If that doesn't work, or tricky, you can update the display name can be updated when the user sets the mouse cursor on the display name widget.

  2. That is a good point. If the user changes the URL, it's surely another display name, hence it should be regenerated.

@peterpardo
Copy link

peterpardo commented Nov 28, 2024

The #1 is doable. But we just need to make sure that the URL they are inputting is valid.

I researched about valid domain names:

  • Only the special character Hyphen "-" is allowed in the domain name, but not at the beginning or at the end
  • Domain names can have a-z (case insensitive) and 0-9
  • Domain names can only have max 63 characters (not including the TLD)

We need to take this into account.

@peterpardo
Copy link

The #1 is doable. But we just need to make sure that the URL they are inputting is valid.

I researched about valid domain names:

  • Only the special character Hyphen "-" is allowed in the domain name, but not at the beginning or at the end
  • Domain names can have a-z (case insensitive) and 0-9
  • Domain names can only have max 63 characters (not including the TLD)

We need to take this into account.

Actually, we don't need to check if the URL is valid. Just discovered that it shows an alert saying that the URL endpoint doesn't resolve.

@peterpardo
Copy link

checkmate-issue-992-demo-1.mp4

@gorkem-bwl Here's the live app test. Let me know if there are things we need to change. Thanks!

@gorkem-bwl
Copy link
Contributor Author

Perfect. That is simple and to the point. Thanks for this!

@peterpardo
Copy link

Cool! Should I proceed to create a PR for this?

@peterpardo
Copy link

I was looking at PRs, I think this issue is already handled. Just waiting for it to be merged into develop and closed.

PR link: feat: create monitor display name, resovles #992 #1221

@gorkem-bwl
Copy link
Contributor Author

Cool! Should I proceed to create a PR for this?

Yes. Thanks for this. CC @ajhollid

@ajhollid
Copy link
Collaborator

Hi @peterpardo ,

Yes there is an open PR for this one, but I am not especially attached to the solution, it's more of a proposal.

If you have a better way in mind, especially with regards to the algorithm for parsing the domain from a string, I would love to have your contribution!

@peterpardo
Copy link

Got it. I was looking at the PR you created, I think we can work around it. We just need to make sure that all edge cases when user types in a domain name is taken into account.

@peterpardo
Copy link

checkmate-issue-992-demo-2.mp4

Hello @ajhollid ,

I used the branch that you have right now for this issue because I think it's better with the one I have right now and it catches most of the edge cases when typing in the domain name.

I just made some minor tweaks around the parseDomainName util you created. Let me know if this works for you.

I attached the live test of the app here @gorkem-bwl.

Thank you!

@peterpardo
Copy link

checkmate-issue-992-demo-2.mp4
Hello @ajhollid ,

I used the branch that you have right now for this issue because I think it's better with the one I have right now and it catches most of the edge cases when typing in the domain name.

I just made some minor tweaks around the parseDomainName util you created. Let me know if this works for you.

I attached the live test of the app here @gorkem-bwl.

Thank you!

When the URL has a subdomain, e.g. "api.test.com", I just joined the two strings: api.test.com -> "Api Test". Let me know if you want this behavior for these kinds of cases.

@gorkem-bwl
Copy link
Contributor Author

It looks great to me @peterpardo .

When the URL has a subdomain, e.g. "api.test.com", I just joined the two strings: api.test.com -> "Api Test". Let me know if you want this behavior for these kinds of cases

Yes, this is a good approach. Thanks.

@peterpardo
Copy link

Awesome! Should I proceed to create a PR for this?

@ajhollid
Copy link
Collaborator

@peterpardo sure that sounds like a good approach to me.

If you want to build off my PR feel free to either branch off it and create a new PR or just push commits to this branch, either is fine 👌

@peterpardo
Copy link

Got it! Quick question, is this issue considered a feat or a fix to a bug?

@ajhollid
Copy link
Collaborator

Got it! Quick question, is this issue considered a feat or a fix to a bug?

I think a feature as it is adding new functionality

@peterpardo
Copy link

Done! Created a PR for this, #1234, and is ready for review. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

5 participants