citycoins / ui

Basic UI components for interacting with the CityCoin contract. Requires Stacks Wallet for Web.
https://minecitycoins.com
GNU General Public License v3.0
28 stars 13 forks source link

Replace substr() method with substring() #146

Closed whoabuddy closed 2 years ago

whoabuddy commented 2 years ago

Is your feature request related to a problem? Please describe.

The address component uses the substr() method which is deprecated.

Describe the solution you'd like

Any usage of substr() should be replaced with substring().

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/substr https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/substring

phershbe commented 2 years ago

I'm trying to get experience and found this on an aggregator listing good first issue tags. I can work on this if you're accepting contributors.

whoabuddy commented 2 years ago

Sure, that'd be great! Basic process would be to:

Happy to help with any questions along the way.

If you're on Discord you can join the CityCoins server as well - there are a few areas for builders.

phershbe commented 2 years ago

@whoabuddy Awesome, thank you, I'll get it in later today!!

phershbe commented 2 years ago

@whoabuddy sorry about the delay, something came up yesterday. I'm working on it now. So when I search in the main branch of the unforked repository for "substr()" in the search bar, it brings up instances existing in the branch 1a620c0596. When I search for "substr()" in my fork, it doesn't find any results. Should I have forked branch 1a620c0596 instead of the main branch?

whoabuddy commented 2 years ago

You should checkout the develop branch and create the PR from there, it's the main one that has the most recent code, and once enough changes are finalized it's released to main.

As far as the search I think it's a simple fix: substr() won't match anything because the commands contain values between ( and ), e.g. {addr.substr(0, 5)}...{addr.substr(addr.length - 5)}. If you search for just substr instead you'll see the components that use it: https://github.com/citycoins/ui/search?q=substr

randr000 commented 2 years ago

@phershbe are you still having trouble with this? If you are, I can give it a try. Thanks.

phershbe commented 2 years ago

@randr000 I'm still working on it, I don't like giving up on things. I'm still trying to figure out why substr won't show up in the search on my fork.

phershbe commented 2 years ago

@whoabuddy that's good to know about searching for substr and not substr(), that's a stupid mistake and it's good to learn from. It still finds substr(), I probably searched both ways before too. In any case, I'm still having problems.

I've worked a little bit on some other open source projects and have never had this problem; there seems to be something really simple that I am not understanding in this case.

Here are some screenshots:

Searching in the unforked repository:

searchsuccess

The fork that I created:

myfork

Searching in my forked repository:

searchfailure
whoabuddy commented 2 years ago

One option is to delete the fork and start over - sometimes something funny happens between and a fresh start eliminates any weirdness.

If I couldn't find it via the search then I'd look for those components in their directories and see if the code matches what I expect to see, e.g. https://github.com/citycoins/ui/blob/develop/src/components/profile/Address.js#L11-L13

Does that component/folder structure look the same as your fork? If not, you may be on the wrong branch?

phershbe commented 2 years ago

@whoabuddy Thank you for your patience, I'm learning a lot here and hopefully can get this resolved quickly.

Deleting the fork and starting over didn't work.

So in the search for substr in my fork, the issues tab was selected in the search. When I changed the tab to code, it gave me "Sorry, forked repositories are not currently searchable.". Okay so I searched substr again in the unforked repository and it found five files:

src/components/Address.js src/components/Tx.js src/components/TxStatus.js src/lib/transactions.js src/components/CityCoinTxList.js

Clicking on them, however, shows me files in the 10620c0596 branch. I then searched for them in the develop branch by clicking through the components and lib folders and couldn't find most of them (Tx.js, TxStatus.js, transactions.js, CityCoinTxList.js) anywhere except for the Address.js file that you mentioned in the previous comment. My logic there was that if I could find them by clicking through folders, I could still go in and change them in my fork.

Am I correct in my reasoning and results above?

whoabuddy commented 2 years ago

@phershbe No problem! I still suspect what's happening here is related to the default branch main vs develop where the most recent changes live. The paths you have listed above are linked to commit 1a620c059684316e6c7acebf9036164bbf061876, which is definitely a much older one.

Screenshot from 2022-06-07 06-00-17

The search also isn't working as expected for me, I ended up using the search in VS Code to find which files: it's just the LinkTx.js and Address.js components.

image

Depending on how you're accessing things (website, command-line, etc) there are a few steps to make sure you see the right files. Here are links to the two components on the develop branch:

https://github.com/phershbe/ui/blob/develop/src/components/profile/Address.js https://github.com/phershbe/ui/blob/develop/src/components/common/LinkTx.js

You can also sync your fork beforehand as it's two commits behind.

chhetri28 commented 2 years ago

Hey @whoabuddy I've replaced the substr() method with substring() on both of the file.

I'm new to this, could you please tell what's the next step?

phershbe commented 2 years ago

@whoabuddy Awesome, thank you for your help and the detail in answering my questions. Yeah I kind of thought about searching in Visual Studio Code before, I didn't think it would give me different results or something, that's great advice though.

These kinds of challenges are good because I learn a lot from them, it took some time and effort from you though to help so thank you.

I'm going to browse through the issues and see what else I could help with.

phershbe commented 2 years ago

@chhetri28 @whoabuddy I requested to work on this and was assigned it and did a lot of work to figure it out, I should probably be the person submitting the pull request unless somebody says otherwise. Another pull request was made randomly following information that was posted on here answering my questions. I'm trying to get experience and proof of experience. It's not a big deal but I should get credit for doing the work in the form of hopefully an accepted pull request.