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

EZP-30525: Convert Browse Tab as a standalone ReactJS module #187

Merged
merged 2 commits into from May 13, 2019
Merged

EZP-30525: Convert Browse Tab as a standalone ReactJS module #187

merged 2 commits into from May 13, 2019

Conversation

sunpietro
Copy link
Contributor

https://jira.ez.no/browse/EZP-30525

Things done

  • converted the Browse tab into a standalone module,
  • implemented the render as asson as possible approach. The missing content meta will be loaded in the meantime when displaying partially loaded content meta preview. It reduces a delay between selecting a content and displaying the content meta preview.

@sunpietro sunpietro self-assigned this May 10, 2019
@sunpietro sunpietro mentioned this pull request May 10, 2019
Closed
dew326
dew326 reviewed May 10, 2019
View reviewed changes
src/modules/udw/common/select-content-button/select.content.button.component.js Outdated
}).isRequired,
onSelect: PropTypes.func.isRequired,
onDeselect: PropTypes.func.isRequired,
isSelected: PropTypes.bool.isRequired,
Copy link
Member

Choose a reason for hiding this comment

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

It has default so it's not required.

src/modules/udw/common/selected-content/selected.content.item.component.js Outdated
let icon = null;

if (contentTypeIdentifier) {
icon = <Icon name={contentTypeIdentifier} extraClasses="c-selected-content-item__icon ez-icon--small" />;
Copy link
Member

Choose a reason for hiding this comment

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

Why not to use the ContentTypeIconComponent?

src/modules/udw/services/bookmark.service.js
method: 'HEAD',
});
const bookmarkedStatusCode = 200;
const notBookmarkedStatusCode = 404;
Copy link
Member

Choose a reason for hiding this comment

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

Can we change this and use always 200 with the response { bookmarked: true|false}?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, but I'll keep it like this, Because we need changes on the backend side as well. This work has not been planned yet.

@sunpietro sunpietro requested a review from dew326 May 10, 2019 11:47
@sunpietro sunpietro changed the base branch from ezp-30530-base-udw to master May 13, 2019 06:53
@sunpietro
Copy link
Contributor Author

ping @dew326

tischsoic
tischsoic reviewed May 13, 2019
View reviewed changes
src/modules/udw/common/select-content-button/select.content.button.component.js
);

useEffect(() => {
checkCanSelectContent(location, toggleEnabledState);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we setIsSelectContentEnabled(false) before checkCanSelectContent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not the best idea in the world, because it will cause extra re-renders of a component. Another effect is basing on changes of isSelectContentEnabled flag.

src/modules/udw/common/selected-content/selected.content.component.js Outdated
import PopupComponent from '../../../common/tooltip-popup/tooltip.popup.component';
import { createCssClassNames } from '../../../common/css-class-names/css.class.names';

const noConfirmedContentTitle = Translator.trans(
Copy link
Contributor

Choose a reason for hiding this comment

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

some variables holding translations text use UPPER_CASE, and here we have camelCase, is this intended? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. I'll correct that here and there.

@sunpietro sunpietro requested a review from tischsoic May 13, 2019 13:11
@sunpietro sunpietro merged commit c1ebd42 into master May 13, 2019
tischsoic added a commit that referenced this pull request May 30, 2019
@tischsoic
Revert "EZP-30525: Convert Browse Tab as a standalone ReactJS module (#…
b01509f 
…187)"

This reverts commit c1ebd42.
@tischsoic tischsoic mentioned this pull request May 30, 2019
Merged
tischsoic added a commit that referenced this pull request Jun 11, 2019
@tischsoic
Revert new UDW changes (#193)
3de5a42 
* Revert "EZP-30525: Convert Browse Tab as a standalone ReactJS module (#187)"

This reverts commit c1ebd42.

* Revert "EZP-30530: Create a base for the new UDW (#186)"

This reverts commit 2baf3d0.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tischsoic tischsoic tischsoic approved these changes

@dew326 dew326 Awaiting requested review from dew326

Assignees

@sunpietro sunpietro

Labels
Feature Ready for review
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@sunpietro @tischsoic @dew326