Skip to content

rewrite series sales block to react #1439

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
Merged
merged 1 commit into from
Jun 28, 2020
Merged

rewrite series sales block to react #1439

merged 1 commit into from
Jun 28, 2020

Conversation

bahoss
Copy link
Contributor

@bahoss bahoss commented Jun 8, 2020 •
edited by php-coder
Loading

Fix #1329

@bahoss bahoss requested a review from php-coder as a code owner June 8, 2020 06:24
@mystamps-bot
Copy link

mystamps-bot commented Jun 8, 2020 •
edited
Loading

1 Message
📖 @bahoss thank you for the PR! All quality checks have been passed! Next step is to wait when @php-coder will review this code

Generated by 🚫 Danger

php-coder
php-coder requested changes Jun 8, 2020
View reviewed changes
Copy link
Owner

@php-coder php-coder left a comment

Choose a reason for hiding this comment

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

Thank you for the PR!

See the comments. The main issue is that we should make a GET request and obtain the data from a server.

@bahoss bahoss force-pushed the rewrite-series-sales-block-to-React branch from 335256f to 2b6e6e4 Compare June 10, 2020 14:50
php-coder
php-coder requested changes Jun 10, 2020
View reviewed changes
Copy link
Owner

@php-coder php-coder left a comment

Choose a reason for hiding this comment

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

Hi, thank you for the update! See my comments.

Also:

  • ``Fix /account/register: add clarification about using user's e-mail #123` should be written with a space between word and hash. Otherwise GitHub doesn't recognize it. I've fixed PR's title but the commit message should be fixed as well.
  • commit message should start with a type of a change. Example: refactor: rewrite the series sales block to React

@php-coder
Copy link
Owner

P.S. Please, pull the changes from upstream and rebase your branch.

@bahoss bahoss force-pushed the rewrite-series-sales-block-to-React branch from 3a21fb5 to 84e2b49 Compare June 11, 2020 03:23
@bahoss bahoss force-pushed the rewrite-series-sales-block-to-React branch 2 times, most recently from 1863af5 to e40fb68 Compare June 14, 2020 15:49
php-coder
php-coder requested changes Jun 15, 2020
View reviewed changes
Copy link
Owner

@php-coder php-coder left a comment

Choose a reason for hiding this comment

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

I left some comments.

Heh, it's turned out that this task isn't simple.

src/main/frontend/src/components/SeriesSalesList.js Outdated
const { sale, index, l10n } = this.props;
const hasBuyer = !!sale.buyerName;
const hasCondition = !!sale.condition;
const hasDate = !!sale.condition;
Copy link
Owner

Choose a reason for hiding this comment

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

This looks totally wrong.

src/main/frontend/src/components/SeriesSalesList.js Outdated
</a>
)
}
}
Copy link
Owner

Choose a reason for hiding this comment

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

A file should have a newline at the end.

@bahoss bahoss force-pushed the rewrite-series-sales-block-to-React branch from f85470f to 46f7b37 Compare June 16, 2020 06:31
php-coder
php-coder requested changes Jun 16, 2020
View reviewed changes
Copy link
Owner

@php-coder php-coder left a comment

Choose a reason for hiding this comment

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

See my comments.

Also, we need to update the changes and pull the changes from master branch (and resolve a conflict).

src/main/frontend/src/components/SeriesSalesList.js Outdated
class SeriesSaleItem extends React.PureComponent {
render() {
const { sale, index, l10n } = this.props;
const hasBuyer = sale.buyerName !== undefined;
Copy link
Owner

Choose a reason for hiding this comment

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

Why you rewrote !!sale.buyerName with sale.buyerName !== undefined ?

@bahoss bahoss force-pushed the rewrite-series-sales-block-to-React branch 2 times, most recently from 071f755 to f823cee Compare June 16, 2020 11:44
php-coder
php-coder requested changes Jun 18, 2020
View reviewed changes
Copy link
Owner

@php-coder php-coder left a comment

Choose a reason for hiding this comment

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

See my comments. Also, even if I'd like to merge the change, I can't because CI is read and some tests fail. Do you know why they fail?

Let me know if you need my help.

@bahoss bahoss force-pushed the rewrite-series-sales-block-to-React branch from f823cee to f2c13f5 Compare June 21, 2020 09:19
php-coder
php-coder requested changes Jun 21, 2020
View reviewed changes
src/main/java/ru/mystamps/web/feature/site/ResourceUrl.java
@@ -34,7 +34,7 @@
// MUST be updated when any of our resources were modified
public static final String RESOURCES_VERSION = "v0.4.3.12";

// CheckStyle: ignore LineLength for next 15 lines
// CheckStyle: ignore LineLength for next 18 lines
Copy link
Owner

Choose a reason for hiding this comment

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

Why 18? You added a one line, so I'd expect that it should be 16

@php-coder
Copy link
Owner

  1. TypeError: Cannot find function map in object

This error happens because when we make a GET query to a server, we got HTML page back as there is no API yet. We should turn the component off until we have implementation on a backend.

I added USE_REACT feature toggle in 3ebd680 commit.

@php-coder
Copy link
Owner

Please, update master and rebase your PR first.

@bahoss bahoss force-pushed the rewrite-series-sales-block-to-React branch from 4b1b731 to e58b87a Compare June 22, 2020 14:16
@bahoss bahoss force-pushed the rewrite-series-sales-block-to-React branch from cdb10f7 to cf3747a Compare June 28, 2020 07:16
@php-coder
Copy link
Owner

The error messages on the PR page are outdated because danger failed to install: #1453

@php-coder
Copy link
Owner

There also many errors like this:

com.gargoylesoftware.htmlunit.ScriptException: Invariant Violation: Minified React error #200; visit https://reactjs.org/docs/error-decoder.html?invariant=200 for the full message or use the non-minified dev environment for full errors and additional helpful warnings. (http://127.0.0.1:8080/public/react-dom/16.8.6/umd/react-dom.production.min.js#13)

I was able to reproduce it locally in a browser. In development mode the error is the following:

Invariant Violation: Target container is not a DOM element.

Why it happens? The target <div> for SeriesSalesList is rendered only when USE_REACT is active:

<div id="series-sales-list" sec:authorize="hasAuthority('VIEW_SERIES_SALES')" togglz:active="USE_REACT"></div>

But it isn't activated by default and the div isn't here. So, when react tries to render the content into the div, it couldn't find it and fails. In order to fix this, the logic for rendering the component also should be wrapped into toggle:active.

@php-coder
Copy link
Owner

In order to fix this, the logic for rendering the component also should be wrapped into toggle:active.

I've submitted PR to you: bahoss#1

@php-coder
Copy link
Owner

The error messages on the PR page are outdated because danger failed to install: #1453

The issue has been fixed.

@php-coder
Copy link
Owner

@bahoss Could you squash the commits into one and rebase on top of the master (need to update it first)?

@bahoss bahoss force-pushed the rewrite-series-sales-block-to-React branch from 9cbb9c4 to 956dee7 Compare June 28, 2020 11:10
php-coder
php-coder approved these changes Jun 28, 2020
View reviewed changes
Copy link
Owner

@php-coder php-coder left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you! 👍

@php-coder php-coder merged commit cefbc0e into php-coder:master Jun 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@php-coder php-coder php-coder approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

/series/{id}: rewrite series sales block to React
3 participants
@bahoss @mystamps-bot @php-coder