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

Fix #535 & #542 & #541 & #409 & #529 & #549 & #590 & #579 & #568 & #506 & #557 & #574 & #594 #592

Merged
merged 29 commits into from
Feb 23, 2018

Conversation

jonnetanninen
Copy link
Contributor

@jonnetanninen jonnetanninen commented Feb 12, 2018

Solves multiple small issues: #535 & #542 & #541 & #409 & #529 & #549 & #590 & #579 & #568 & #506 & #557 & #574 & #594

@ghost ghost assigned jonnetanninen Feb 12, 2018
@ghost ghost added the in progress label Feb 12, 2018
@codecov-io
Copy link

codecov-io commented Feb 12, 2018

Codecov Report

Merging #592 into master will decrease coverage by 0.22%.
The diff coverage is 30.76%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #592      +/-   ##
==========================================
- Coverage   24.96%   24.74%   -0.23%     
==========================================
  Files         121      121              
  Lines        2471     2514      +43     
  Branches      420      433      +13     
==========================================
+ Hits          617      622       +5     
- Misses       1540     1566      +26     
- Partials      314      326      +12
Impacted Files Coverage Δ
src/components/HearingList.js 46.66% <ø> (ø) ⬆️
src/components/admin/HearingFormStep3.js 4.54% <ø> (ø) ⬆️
src/views/Hearing/HearingContainer.js 47.36% <ø> (ø) ⬆️
...c/components/Hearing/Section/SectionClosureInfo.js 66.66% <ø> (ø) ⬆️
src/components/SocialBar/Facebook.js 50% <0%> (-12.5%) ⬇️
src/App.js 0% <0%> (ø) ⬆️
src/components/Header/index.js 59.37% <0%> (-3.96%) ⬇️
src/views/Info/index.js 0% <0%> (ø) ⬆️
src/components/SocialBar/Twitter.js 100% <100%> (+20%) ⬆️
src/components/Hearing/Section/SectionImage.js 100% <100%> (ø) ⬆️
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ba69a09...6aaab97. Read the comment docs.

@jonnetanninen jonnetanninen changed the title WIP: Fix #535 & #542 & #541 & #409 WIP: Fix #535 & #542 & #541 & #409 & #529 Feb 12, 2018
@jonnetanninen jonnetanninen changed the title WIP: Fix #535 & #542 & #541 & #409 & #529 WIP: Fix #535 & #542 & #541 & #409 & #529 & #549 Feb 12, 2018
@jonnetanninen jonnetanninen changed the title WIP: Fix #535 & #542 & #541 & #409 & #529 & #549 WIP: Fix #535 & #542 & #541 & #409 & #529 & #549 & #590 Feb 13, 2018
@jonnetanninen jonnetanninen changed the title WIP: Fix #535 & #542 & #541 & #409 & #529 & #549 & #590 Fix #535 & #542 & #541 & #409 & #529 & #549 & #590 & #579 Feb 13, 2018
@jonnetanninen jonnetanninen changed the title Fix #535 & #542 & #541 & #409 & #529 & #549 & #590 & #579 Fix #535 & #542 & #541 & #409 & #529 & #549 & #590 & #579 & #568 Feb 14, 2018
@jonnetanninen jonnetanninen changed the title Fix #535 & #542 & #541 & #409 & #529 & #549 & #590 & #579 & #568 Fix #535 & #542 & #541 & #409 & #529 & #549 & #590 & #579 & #568 & #506 Feb 14, 2018
@jonnetanninen jonnetanninen changed the title Fix #535 & #542 & #541 & #409 & #529 & #549 & #590 & #579 & #568 & #506 Fix #535 & #542 & #541 & #409 & #529 & #549 & #590 & #579 & #568 & #506 & #557 Feb 14, 2018
@jonnetanninen jonnetanninen changed the title Fix #535 & #542 & #541 & #409 & #529 & #549 & #590 & #579 & #568 & #506 & #557 Fix #535 & #542 & #541 & #409 & #529 & #549 & #590 & #579 & #568 & #506 & #557 & #574 Feb 14, 2018
Copy link
Contributor

@Rikuoja Rikuoja left a comment

Choose a reason for hiding this comment

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

Damn, a merge conflict appears!

@jonnetanninen jonnetanninen changed the title Fix #535 & #542 & #541 & #409 & #529 & #549 & #590 & #579 & #568 & #506 & #557 & #574 Fix #535 & #542 & #541 & #409 & #529 & #549 & #590 & #579 & #568 & #506 & #557 & #574 & #594 Feb 19, 2018
Copy link
Contributor

@Rikuoja Rikuoja left a comment

Choose a reason for hiding this comment

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

A few small issues:

  1. The error message styling should be finalized (increase left marginal, use proper font, get rid of the progress bar)
  2. When entering a Hearing, the Hearing name is no longer shown on the page title; instead, the name of the Hearing page is still "Tervetuloa".
  3. The spinner when (re)loading comments should be rendered inside the Mielipiteet section, so that the Mielipiteet header itself (and the count that is known) is visible when the spinner is displayed. This is a visual indication of what is being loaded, i.e. the comments below the header.

@jonnetanninen
Copy link
Contributor Author

Doned!

<LoadSpinner />
</div>
)}
{showCommentList && !this.state.showLoader &&
Copy link
Contributor

@Rikuoja Rikuoja Feb 21, 2018

Choose a reason for hiding this comment

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

I am thinking here we actually shouldn't require !this.state.showLoader.

This is because we would like the comment list header and the order filter selection to be visible during loading of comments, so the user can see what is being fetched and in what order.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually this does not seem to make a difference anyway as the old condition showCommentList will be false while fetching as the sectionComments is empty. I think this might be appropriate as showing the filter selector while fetching would allow the user to spam API calls by rapidly changing the filter...

<h2>
<FormattedMessage id="comments" />
<div className="commenticon">
<Icon name="comment-o" />&nbsp;{get(sectionComments, 'count') ? sectionComments.count : '0'}
Copy link
Contributor

Choose a reason for hiding this comment

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

Here, we shouldn't use sectionComments.count but, rather, section.n_comments. The former is zero when fetching, but the latter always contains the corrent count. Very seldom do we need sectionComments.count anywhere.

Copy link
Contributor

@Rikuoja Rikuoja left a comment

Choose a reason for hiding this comment

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

Looks good apart from the two small comments!

@Rikuoja Rikuoja merged commit ebd8ba2 into master Feb 23, 2018
@ghost ghost removed the in progress label Feb 23, 2018
@ranta ranta deleted the feature/small-improvements branch November 23, 2021 10:31
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.

3 participants