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

feat(admin-trades): Implementing API and UI #3049

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

doga-foton
Copy link

@doga-foton doga-foton commented Oct 29, 2021

It's a work in progress.

Related to:
OV-1262
OV-1261

API:

  • API endpoint has been added
  • Query to get trades with relational data to list on UI

UI:

  • Menu item has been added
  • View has been added

image

I would like to get feedback about;

  • If the implementation of the UI applied to guidelines.
  • I needed to add two controllers on /trade/exchange and /trade/exchange-irec packages.
  • Existing trade.service has been used, and made getAll method as public, should I create a service only for this case?
  • Folder / file structure

@CLAassistant
Copy link

CLAassistant commented Oct 29, 2021

CLA assistant check
All committers have signed the CLA.

@doga-foton doga-foton marked this pull request as draft October 29, 2021 08:42
Copy link
Contributor

@alexworker23 alexworker23 left a comment

Choose a reason for hiding this comment

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

In general: the implementation of the UI applies to guidelines ✅

const formatTrades: TFormatTrades = ({ trades, allDevices }) => {
const formattedTrades: FormattedTrade[] = [];

trades?.forEach((trade) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

const formattedTrades: FormattedTrade[] = trades.map will better fit here

date: t('admin.trades.date'),
buyer: t('admin.trades.buyer'),
seller: t('admin.trades.seller'),
energy: 'Energy',
Copy link
Contributor

Choose a reason for hiding this comment

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

please don't forget to localize this

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

Successfully merging this pull request may close these issues.

3 participants