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.

Sign up for GitHub

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

Jump to bottom

feat: add components for nano contract transactions [9] #456

Merged
merged 2 commits into from
May 28, 2024
Merged

feat: add components for nano contract transactions [9] #456

merged 2 commits into from
May 28, 2024

Conversation

alexruzenhack
Copy link
Contributor

Motivation

This is a PR on a sequence to make the review easier.

Acceptance Criteria

  • Should add all components to be used on nano contract transactions screen

Note

It depends on:

Security Checklist

  • Make sure you do not include new dependencies in the project unless strictly necessary and do not include dev-dependencies as production ones. More dependencies increase the possibility of one of them being hijacked and affecting us.

@alexruzenhack alexruzenhack self-assigned this Apr 1, 2024
This was referenced Apr 1, 2024
Merged
Merged
@alexruzenhack alexruzenhack changed the base branch from feat/nano-contract-list-component to feat/nc-add-icons April 5, 2024 17:38
@alexruzenhack alexruzenhack changed the title feat: add components for nano contract transactions feat: add components for nano contract transactions [9] Apr 8, 2024
andreabadesso
andreabadesso requested changes Apr 10, 2024
View reviewed changes
src/utils.js Outdated Show resolved Hide resolved
src/components/ModalBase.component.js Outdated Show resolved Hide resolved
@alexruzenhack alexruzenhack force-pushed the feat/nc-add-icons branch 2 times, most recently from 37fa2b3 to 06e563d Compare May 13, 2024 18:23
@alexruzenhack alexruzenhack requested review from andreabadesso, tuliomir and r4mmer and removed request for tuliomir May 14, 2024 16:04
r4mmer
r4mmer previously approved these changes May 14, 2024
View reviewed changes
andreabadesso
andreabadesso requested changes May 21, 2024
View reviewed changes
locale/pt-br/texts.po Outdated Show resolved Hide resolved
locale/ru-ru/texts.po Outdated Show resolved Hide resolved
locale/pt-br/texts.po Outdated Show resolved Hide resolved
src/components/EditInfoContainer.component.js Outdated Show resolved Hide resolved
src/components/ModalBase.component.js Outdated Show resolved Hide resolved
src/components/TextLabel.component.js Outdated Show resolved Hide resolved
src/components/TextValue.component.js Outdated Show resolved Hide resolved
src/utils.js
@@ -392,3 +393,5 @@ export function combineURLs(baseURL, relativeURL) {
export const isPushNotificationAvailableForUser = (state) => (
state.pushNotification.available && state.pushNotification.deviceRegistered
);

export const getTimestampFormat = (timestamp) => moment.unix(timestamp).format(t`DD MMM YYYY [•] HH:mm`)
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing docstring

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, is this localized?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is internationalized but regarding localization for en-us I didn't prepared it, I'm following the pattern we already have. Maybe we should open an issue just for this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please create the issue and link here?

@alexruzenhack alexruzenhack force-pushed the feat/nc-add-icons branch 2 times, most recently from 1e1e19b to 6c9c73e Compare May 23, 2024 16:30
@alexruzenhack alexruzenhack force-pushed the feat/nc-add-components branch 2 times, most recently from ea8deb9 to 2c4494b Compare May 23, 2024 22:23
@alexruzenhack alexruzenhack dismissed r4mmer’s stale review May 23, 2024 22:25

Because Abadesso has come back from vacation.

pedroferreira1
pedroferreira1 reviewed May 28, 2024
View reviewed changes
src/utils.js
*
* @returns {string} formatted timestamp
*/
export const getTimestampFormat = (timestamp) => moment.unix(timestamp).format(t`DD MMM YYYY [•] HH:mm`)
Copy link
Member

Choose a reason for hiding this comment

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

This is copying what we have in the src/models.js file to the utils. You should at least change the models file to use the utils as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, never mind. I tried just yet to call the function from src/models and lint has cried about a cycle, take a look:
image

Can you let it this way and I open a KTLO just to solve it? Utils should not be model dependent, it must be independent as much as possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here is the issue:
#491

Base automatically changed from feat/nc-add-icons to master May 28, 2024 14:24
@alexruzenhack alexruzenhack merged commit c744c93 into master May 28, 2024
2 checks passed
@alexruzenhack alexruzenhack deleted the feat/nc-add-components branch May 28, 2024 15:59
andreabadesso pushed a commit that referenced this pull request Jun 24, 2024
@alexruzenhack @andreabadesso
feat: add components for nano contract transactions [9] (#456)
31931fd 
* feat(nc): add components
@alexruzenhack alexruzenhack mentioned this pull request Oct 3, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pedroferreira1 pedroferreira1 pedroferreira1 approved these changes

@andreabadesso andreabadesso andreabadesso approved these changes

@r4mmer r4mmer r4mmer left review comments

Assignees

@alexruzenhack alexruzenhack

Labels
None yet
Projects
Hathor Network
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@alexruzenhack @pedroferreira1 @andreabadesso @r4mmer