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

Implement new release banner - Closes #2144 #2300

Merged
merged 25 commits into from Aug 7, 2019

Conversation

@massao
Copy link
Contributor

commented Aug 1, 2019

What issue have I solved?

#2144

How have I implemented/fixed it?

  • Refactor flashMessage toolbox component to be more generic;
  • Created base header to render signInHeader or topBar component;
  • Created flashMessageHolder to be easier to render new flash messages;
  • Pass data from the IPC to new release banner;

The release summary is fetched from the release note and consists of the content of the first html tag.

The following items will be done in a followup ticket #2317 .

  • Open modal with IPC data through release banner;
  • Start update through click on dialog;
  • Show toaster that update started;

How has this been tested?

Change version on package.json, build the application an run with electront o see that the banner shows up.

Review checklist

@massao massao self-assigned this Aug 1, 2019

@massao massao added this to Pull Requests in Version 1.20.0 via automation Aug 1, 2019

};

FlashMessage.displayName = 'FlashMessage';
FlashMessage.Text = Text;
FlashMessage.Button = Button;

This comment has been minimized.

Copy link
@slaweet

slaweet Aug 1, 2019

Member

Awesome 🏆

@massao massao force-pushed the 2144-implement-new-release-banner branch 3 times, most recently from 11879de to cf5a04e Aug 1, 2019

@massao massao force-pushed the 2144-implement-new-release-banner branch from bb1dc8b to b2d717c Aug 2, 2019

massao added some commits Aug 2, 2019

@massao massao force-pushed the 2144-implement-new-release-banner branch from b2d717c to 981d1ea Aug 3, 2019

@massao massao force-pushed the 2144-implement-new-release-banner branch from 981d1ea to 8e29375 Aug 3, 2019

@massao massao changed the title Implement new release banner and dialog - Closes #2144 Implement new release banner Aug 5, 2019

@massao massao marked this pull request as ready for review Aug 5, 2019

@massao massao requested a review from slaweet Aug 5, 2019

@slaweet
Copy link
Member

left a comment

  • 🐛 The release notes from Github can contain HTML that is escaped (see screenshot)
  • 🐛 In the release notes, the first line is the title of the first section (usually "Fixed bugs" or "Implemented enhancements"), It should IMO show the second line. (see "body" in https://api.github.com/repos/LiskHQ/lisk-hub/releases)
  • 🐛 "Read more" seems slightly vertically miss-aligned with the text.
    Screenshot 2019-08-05 at 10 05 56
  • On the dark splash screen it looks a bit weird, maybe bottom border would help. Please check with design team.
    Screenshot 2019-08-05 at 10 05 23

Otherwise, it works well. I'm still going through the code...

@massao

This comment has been minimized.

Copy link
Contributor Author

commented Aug 5, 2019

  • 🐛 The release notes from Github can contain HTML that is escaped (see screenshot)

I'm checking into it.

By what I discussed with @reyraa for future releases we are going to have the first line being a heading with the summary. But we can check other alternatives with him, if needed.

  • 🐛 "Read more" seems slightly vertically miss-aligned with the text.
    Screenshot 2019-08-05 at 10 05 56

This seems to be because the button has a font-size: 15px; and the text has font-size: 13px, because it's using display: flex; align-items: center; so it should be ok.

  • On the dark splash screen it looks a bit weird, maybe bottom border would help. Please check with design team.
    Screenshot 2019-08-05 at 10 05 23

Will check and update this comment with the conclusion.
Design team will update invision with one version of the splashscreen with the banner. https://projects.invisionapp.com/d/main#/console/17570736/377372026/preview

@slaweet

This comment has been minimized.

Copy link
Member

commented Aug 5, 2019

  • 🐛 Account initialization is not displayed anymore (I saw it in the HTML, but CSS makes it not visible somehow)

Screenshot 2019-08-05 at 10 56 18

@slaweet

This comment has been minimized.

Copy link
Member

commented Aug 5, 2019

By what I discussed with @reyraa for future releases we are going to have the first line being a heading with the summary. But we can check other alternatives with him, if needed.

The first line with summary sounds good. Will there be some formatting, like bold or H1-6 @reyraa ? IMO it makes sense to have the summary somehow highlighted on Github as one of the most important parts. Can we already do it for the latest release?
https://github.com/LiskHQ/lisk-hub/releases/tag/v1.19.1 So that we can test what does it look like now and not after the release.

Show resolved Hide resolved src/utils/newRelease.js Outdated
Show resolved Hide resolved src/components/newReleaseMessage/newReleaseMessage.js Outdated

massao added some commits Aug 5, 2019

@massao massao force-pushed the 2144-implement-new-release-banner branch from 980b57e to 818b4de Aug 5, 2019

@reyraa

This comment has been minimized.

Copy link
Member

commented Aug 5, 2019

@slaweet @massao Yes, let's use a heading for the summary so it's easier read and we can grab the summary easily.

@massao massao requested a review from slaweet Aug 5, 2019

@reyraa

This comment has been minimized.

Copy link
Member

commented Aug 5, 2019

@massao I jazzed up this release note so you can test easily.
https://github.com/LiskHQ/lisk-hub/releases/tag/v1.19.1
Let me know if it works for you.

@slaweet

This comment has been minimized.

Copy link
Member

commented Aug 5, 2019

  • 🐛 Currently, it displays only the "Release goal" text. Do we want to change the code or the release notes format?

Screenshot 2019-08-05 at 15 39 29

@slaweet

This comment has been minimized.

Copy link
Member

commented Aug 5, 2019

When I tried to change it to display the actual text, I found out 🐛 it doesn't wrap the text on multiple lines if too long (or small screen)
Screenshot 2019-08-05 at 15 42 45

massao added some commits Aug 5, 2019

Show resolved Hide resolved src/utils/htmlParser.js Outdated

massao added some commits Aug 5, 2019

@massao

This comment has been minimized.

Copy link
Contributor Author

commented Aug 5, 2019

🐛 Currently, it displays only the "Release goal" text. Do we want to change the code or the release notes format?

For now I set the releaseSummary as being the first h4 on the release notes.

When I tried to change it to display the actual text, I found out 🐛 it doesn't wrap the text on multiple lines if too long (or small screen)

Fixed the wrapping issue, but had to disable show/hide animations, as it was relying on a specific height.

massao added some commits Aug 5, 2019

@massao massao force-pushed the 2144-implement-new-release-banner branch from 8d392a5 to a50256c Aug 5, 2019

@slaweet

slaweet approved these changes Aug 5, 2019

Copy link
Member

left a comment

Thank you Massao, good job.

@slaweet slaweet requested a review from Efefefef Aug 5, 2019

@reyraa reyraa added this to the Sprint 6 milestone Aug 6, 2019

@slaweet slaweet requested a review from yasharAyari Aug 6, 2019

@massao massao changed the title Implement new release banner Implement new release banner - Closes #2144 Aug 6, 2019

@Efefefef
Copy link
Contributor

left a comment

Great 👍

@Efefefef Efefefef added the ready label Aug 7, 2019

@massao massao removed the request for review from yasharAyari Aug 7, 2019

@massao massao merged commit f82e042 into development Aug 7, 2019

3 checks passed

Jenkins e2e tests e2e tests passed
Details
Jenkins test deployment Commit was deployed to test
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details

Version 1.20.0 automation moved this from Pull Requests to Merged Pull Requests Aug 7, 2019

@massao massao deleted the 2144-implement-new-release-banner branch Aug 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.