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

Trello 19 #31

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Trello 19 #31

wants to merge 11 commits into from

Conversation

ZiyedB
Copy link
Collaborator

@ZiyedB ZiyedB commented Mar 14, 2021

Draft PR ,only look at the last commit. We need to get the other PRs reviewed and approved to merge them.

Basic so far :
ezgif-2-5879bce76a3b

Will add the :

  • Add
  • Enable/Disable
  • Remove
  • Edit
  • Save

{chain.name}
</div>
);
});
Copy link
Collaborator

@Frenkiee Frenkiee Mar 15, 2021

Choose a reason for hiding this comment

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

A bit nicer:

 const allChains = chains
    .filter((chain) => !searchChain)
    .filter((chain) => chain.name.includes(searchChain.trim().toLocaleLowerCase()))
    .map((chain) => (
        <div
          className={`focus:outline-none py-4 capitalize cursor-pointer ${
            chain.name === activeChain?.name ? 'font-bold text-primary' : ''
          }`}
          role="button"
          onClick={() => setActiveChain(chain)}
          onKeyPress={() => setActiveChain(chain)}
          tabIndex={0}
        >
          {chain.name}
        </div>
      ));

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, I would try to move out the div view and present it as a component

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The filter like that wouldn't actually work but yeah simplifying it and moving it to a separate component.

return contract;
})
: 'No Tracked Contracts';

Copy link
Collaborator

Choose a reason for hiding this comment

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

This should also work

const activeContracts =
    activeChain && activeChain.tracked_contracts.length
      ? activeChain.tracked_contracts
      : 'No Tracked Contracts';

project: string;
url: string;
credentials: {
// TODO: Ask potentially BE to have structure camelCase in case or we can just disable the warning in eslint
Copy link
Collaborator

Choose a reason for hiding this comment

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

I've changed eslint settings in Query Builder to display a warning

@@ -0,0 +1,33 @@
// We can remove this when we can hook to the BE and get that data it's just for getting the UX ready for it
const mockDataChain = [
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just mocking the sample we got from Jure

onClick: () => void;
}

const TrackedChain: React.FC<TrackedChainProps> = ({ name, active, onClick }) => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Moved it here, in the same folder, I don't think it's gonna be used anywhere else so no need to put it inside components

</div>
);
});
.filter((chain) => chain.name.includes(searchChain.trim().toLocaleLowerCase()) || activeChain?.name === chain.name)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just this filter works actually no need to check for if defined or null. Also added the fact that if a chan is already active you might still want to see it despite searching

@ZiyedB
Copy link
Collaborator Author

ZiyedB commented Apr 1, 2021

Screenshot 2021-04-01 at 16 14 24

Screenshot 2021-04-01 at 16 14 21

Screenshot 2021-04-01 at 16 07 20

/* eslint-disable @typescript-eslint/explicit-module-boundary-types */

// Implementation : https://github.com/paralink-network/paralink-node/blob/master/src/api/contracts.py
const getAllContracts = () => axiosInstance.get<Contract[]>(`${contractsApi}`);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

FYI @frisitano @jbargu I've tried some of the BE Apis and had some feedback :

  1. https://github.com/paralink-network/paralink-node/blob/master/src/api/contracts.py#L47 POST & PUT should return the contract response instead of just OK ( we'd need the latest from the DB, especially in the POST to have the ID without refreshing the page or refetching all of the contracts )

  2. Error when trying to update the status of an address, we might want to fix that

Screenshot 2021-04-01 at 15 52 43

  1. Should we be able to edit an address after it was created or saved ? cause we don't have a "setAddress" or a modify endpoint. Also if we were to add one, let's just do it as "updateContract" endpoint and remove the status one.

  2. Error when deleting. It actually works, but return a 500 as there is an error after deleting :

Screenshot 2021-04-01 at 15 48 21

Copy link
Member

Choose a reason for hiding this comment

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

Hi @ZiyedB

I have made some modifications to the API contract on this development branch - refactor_models .

This refactor modifies the API such that contracts are identified by a unique (chain, address) tuple.

  1. Under this new API you should not need any additional information returned by the API as the contract id has been removed.

  2. I am not experiencing the same issue as you. I suspect your .env file may be referencing the celery broker incorrectly. Could you ensure it contains CELERY_BROKER_URL=amqp://guest:guest@rabbitmq:5672 please?

  3. No we should not be able to update the address. A different address would represent a different contract.

  4. Fixed.

Could you take a look at the API and let me know your feedback. I will hold fire on raising a PR to see if we need to make any modifications to the API for the front end. In any case I still need to develop some tests for the REST endpoints.

Copy link
Member

@frisitano frisitano Apr 27, 2021

Choose a reason for hiding this comment

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

I forgot to mention that you will have to delete the docker volume associated with the paralink Postgres container before executing the dev branch above.

@@ -0,0 +1,8 @@
import { Contract } from './Contract';
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

FYI @Frenkiee I moved things inside the interfaces folder, I think it might be much easier to share the interfaces across things ( apis / components / utils etc. ) avoiding some circular deps later on

@@ -1,59 +1,82 @@
import React, { useEffect, useState } from 'react';
import { FaCheck, FaTimes, FaTrash } from 'react-icons/fa';
import ContractsApi from '../../api/contracts';
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've done it a slightly differently from you @Frenkiee , I like that format of just importing as named import and then calling the function.

The then / catch & everything else is done directly from the component.

useEffect(() => {
setChains(mockDataChain as Chain[]);
ChainsApi.getAllChains().then((res: any) => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If we can get another chain added

console.log('trackinputRef', trackInputRef);
if (trackInputRef) {
trackInputRef.blur();
}
};

// Save the contract
const saveClick = (): void => {
// save contract as it is
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Before saving we would check that the address is valid or not using some utils. Ticket to come about adding a util to check input address based on the chain

Good example from Jure : https://polkadot.js.org/docs/util-crypto/examples/validate-address/

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 handled in the backend here. You should look out for the error when calling the create_contract endpoint.

// Filter out by the search but also keep the current active in the search
const allChains = chains
.filter((chain) => chain.name.includes(searchChain.trim().toLocaleLowerCase()) || activeChain?.name === chain.name)
.map((chain) => (
<TrackedChain active={chain.name === activeChain?.name} name={chain.name} onClick={() => setActiveChain(chain)} />
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thinking about it @jbargu and your suggestion, I might remove the search and instead have a selector there

Screenshot 2021-04-01 at 16 27 08

Or we could have just one small selectors to select the type 'evm' | 'substrate' ( or both )

@ZiyedB ZiyedB self-assigned this Apr 1, 2021
@ZiyedB
Copy link
Collaborator Author

ZiyedB commented May 9, 2021

FYI @frisitano I've worked on the updates. It's looking good, here is a small gif , you can also try it out. ( added the notifications while at it )

ezgif com-gif-maker

Small thing but not blocking we can address later, as I didn't yet added the "insights" about errors yet but it looks like we'd need a json format for the error. Right now it's an HTML format error.

Screenshot 2021-05-09 at 12 12 01

@ZiyedB ZiyedB marked this pull request as ready for review May 9, 2021 11:14
@ZiyedB ZiyedB added the ready for review PR is ready for review label May 9, 2021
@frisitano
Copy link
Member

frisitano commented May 12, 2021

Hi @ZiyedB I've tried out the UI and it looks great so far. I have updated the error response such that it is json as required, please go ahead and pull the latest. You can see the source code as a reference. I believe the code should be intuitive and self explanatory. In the error scenario you can fetch the error message to display to the user from the "error" field in the json.

@ZiyedB
Copy link
Collaborator Author

ZiyedB commented May 17, 2021

Thanks @frisitano I've added the support for the error , it looks good :
Screenshot 2021-05-17 at 22 57 42
I've added an autoclose after 8s instead, but we can think of persistent error message later in case.

Also for later, I think we should add the following :

  • Errors for other endpoints
  • Potentially just throwing an error ( as right now it's still considered as "response" ) ,for this no need yet, we could potentially try to add a response interceptor to throw an error on our end based on the result, I added a TODO for that, but not sure if it's doable with axios ( need to check )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready for review PR is ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants