Skip to content

Add UI for Currency Conversion transformation#9

Merged
veralimita merged 3 commits intomasterfrom
LITE-26860-currency-conversion-ui
Mar 24, 2023
Merged

Add UI for Currency Conversion transformation#9
veralimita merged 3 commits intomasterfrom
LITE-26860-currency-conversion-ui

Conversation

@irinanic88
Copy link
Copy Markdown
Contributor

No description provided.

@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Mar 23, 2023

CLA assistant check
All committers have signed the CLA.


try {
const overview = await validate('currency_conversion', data);
if (overview.error) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Will the overview always have this structure in case of error? {error: ‘something’}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If we have an error we return {'error': 'error msg'}, but if it is correct we send {'overview': 'overview content'}. If you check the status code, the error ones will be 400 and the correct one 200.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Is this question for @gab832 ?

Comment thread ui/styles/index.css
} No newline at end of file
}

.convert-currency {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Did you decide not to move it to separate CSS file?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I didn't finally, but if you think it's a better idea, I will

<div class="button-container">
<button id="add" class="button">ADD COLUMN</button>
</div>
</main-card>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you!

Comment thread ui/src/pages.js
columns.forEach(column => {
const isSelected = settings && column.name === settings.from.column;

const option = isSelected ? `<option value="${column.id}" selected>${column.name}</option>` : `<option value="${column.id}">${column.name}</option>`;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What do you think about <option value="${column.id}" ${isSelected ? 'selected' : ''>${column.name}</option> ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Looks better, thanks, I'll fix it.

Comment thread ui/src/pages.js
const currencyFullName = currencies[currency];
const isSelected = selectedOption && currency === selectedOption;

const option = isSelected ? `<option value="${currency}" selected>${currency} • ${currencyFullName}</option>` : `<option value="${currency}">${currency} • ${currencyFullName}</option>`;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What do you think about <option value="${currency}" ${isSelected ? 'selected' : ''}>${currency} • ${currencyFullName}?

@veralimita veralimita merged commit 1c703c4 into master Mar 24, 2023
@ffaraone ffaraone deleted the LITE-26860-currency-conversion-ui branch May 4, 2023 06:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants