Reconfigure filter warning messages #234

samwilson · 2019-03-19T02:41:41Z

Set up three filter-message sections (only one of which is
displayed) and modify the form-panel descriptions depending
on the event state.

Bug: T218340

src/AppBundle/Controller/EventController.php

MusikAnimal

If the event has had at least one update, I still see "Statistics have not yet been generated for this event. Calculate totals." in a green alert-success banner at the top. That seems like a bug, or maybe something's wrong on my end? Either way I think should be alert-warning, and also maybe add a bit of spacing above the banner:

Also the new banners are missing some spacing between it and the metadata above it. I don't think it's need to be as much as space as in the mock, but it looks a little odd now:

i18n/en.json

MusikAnimal · 2019-03-20T00:25:10Z

i18n/en.json

+	"error-filters-participants-desc": "Participants filtering applies to all wikis and satisfies all filtering requirements.",
+	"error-filters-participants-none": "No participants supplied",
+	"error-filters-participants-or-categories": "You can filter by Participants or Categories—or combine the two.",
+	"error-filters-wikidata": "To get metrics about <strong>Wikidata</strong>, you must supply Participants. (Categories don’t exist on Wikidata.)",


We've successfully gone this long without any HTML in our messages. Are we sure we really need "Wikidata" to be in bold?

I realize we're using |raw in other places we shouldn't, but the HTML here is a clear indication we're using it.

Yeah, the spec says "Please observe boldfacing", and we don't like piecemeal messages. We could lookup the name Wikidata in the language, and inject it, but even then it's probably not the right thing to do from a l10n perspective.

I'm always confused about the best way to do these things. I wish we could have messages in Markdown!

i18n/qqq.json

app/Resources/views/events/show.html.twig

MusikAnimal · 2019-03-20T00:31:31Z

app/Resources/views/events/show.html.twig

+                <li>{{ msg('error-filters-participants-desc') }}</li>
+                <li>{{ msg('error-filters-categories-desc') }}</li>
+            </ul>
+        </section>


Looks like this <section> group needs to be indented, same with the above
(see also https://twig.symfony.com/doc/2.x/coding_standards.html which of course we don't have to strictly follow :)

Done.

I guess there's no linter for the Twig standards?

src/AppBundle/Model/Event.php

i18n/en.json

src/AppBundle/Controller/EventController.php

src/AppBundle/Model/Event.php

app/Resources/views/events/show.html.twig

MusikAnimal · 2019-03-20T01:12:54Z

Another possible bug (sorry for the flurry of comments): If I have two wikis, en.wikipedia and commons.wikimedia, both with categories, I still see "Event partially configured" even though it should be valid. Perhaps this is because #217 hasn't been merged yet? Overall I think this PR should wait on it... what do you think?

app/Resources/views/events/show.html.twig

samwilson · 2019-03-20T01:53:02Z

If the event has had at least one update, I still see "Statistics have not yet been generated for this event. Calculate totals." in a green alert-success banner at the top. That seems like a bug, or maybe something's wrong on my end? Either way I think should be alert-warning, and also maybe add a bit of spacing above the banner:

Also the new banners are missing some spacing between it and the metadata above it. I don't think it's need to be as much as space as in the mock, but it looks a little odd now:

I was working on the idea that #228 would be done as well, but now that it's not I'll update these. The green box however is in the spec (although the message needs to be updated; I was going to do that in a separate patch along with some other message changes for the two forms — but part of the same task).

I've added some spacing below the metadata.

samwilson · 2019-03-20T02:00:37Z

Another possible bug (sorry for the flurry of comments): If I have two wikis, en.wikipedia and commons.wikimedia, both with categories, I still see "Event partially configured" even though it should be valid. Perhaps this is because #217 hasn't been merged yet? Overall I think this PR should wait on it... what do you think?

Oops, I was failing to actually count the wikisWithoutCats... done now.

samwilson · 2019-03-20T04:43:54Z

Okay, I think this is ready for re-review.

MusikAnimal

One more tiny thing. Looks like master needs to be rebased in, too

MusikAnimal · 2019-03-21T15:14:50Z

src/AppBundle/Model/Event.php

+     * Wikidata is excluded because it can never have categories.
+     * @return Collection Collection of EventWiki objects.
+     */
+    public function getWikisWithoutCategories(): ArrayCollection


The return type declaration also needs to be Collection. I think if there are EventWikis that are persisted but haven't been flushed to the database, the $this->wikis->filter code will return a PersistentCollection, which in turn will cause an exception.

Oops! Of course. :) Done.

MusikAnimal

I'm still seeing the "Statistics have not yet been generated for this even" banner even if they have been generated. And it doesn't hide itself once the update starts. For that I think you just need to give the banner the .event-wiki-stats--empty class (may need to update the styling in events.scss too), which is then handled by https://github.com/wikimedia/eventmetrics/blob/master/app/Resources/assets/js/eventshow.js#L59 I also realize now this CSS class name isn't very clear... that message doesn't necessarily have to do with EventWikis.

I also think whatever banner is at the top really needs some padding above it. A bit too tight up against the metadata part. But we can take care of that later, maybe this was an intentional design choice.

samwilson · 2019-03-22T03:05:03Z

I'm still seeing the "Statistics have not yet been generated for this even" banner even if they have been generated. And it doesn't hide itself once the update starts.

Fixed. It now hides itself when either of the 'update data' links are clicked.

I also think whatever banner is at the top really needs some padding above it. A bit too tight up against the metadata part. But we can take care of that later, maybe this was an intentional design choice.

I've given it 1em.

MusikAnimal · 2019-03-22T15:30:41Z

Looks good to me! Just need to resolve the merge conflict

Set up three filter-message sections (only one of which is displayed) and modify the form-panel descriptions depending on the event state. Bug: T218340

samwilson force-pushed the filtering-ui-T218340 branch 2 times, most recently from 059672b to 866ffe2 Compare March 19, 2019 07:13

samwilson marked this pull request as ready for review March 19, 2019 07:14

samwilson force-pushed the filtering-ui-T218340 branch 3 times, most recently from 622cdf1 to f824657 Compare March 19, 2019 07:33

MaxSem reviewed Mar 20, 2019

View reviewed changes

src/AppBundle/Controller/EventController.php Outdated Show resolved Hide resolved

samwilson force-pushed the filtering-ui-T218340 branch from f824657 to a78304d Compare March 20, 2019 01:00

MusikAnimal requested changes Mar 20, 2019

View reviewed changes

MusikAnimal reviewed Mar 20, 2019

View reviewed changes

src/AppBundle/Model/Event.php Show resolved Hide resolved

MusikAnimal reviewed Mar 20, 2019

View reviewed changes

app/Resources/views/events/show.html.twig Show resolved Hide resolved

MusikAnimal reviewed Mar 20, 2019

View reviewed changes

app/Resources/views/events/show.html.twig Outdated Show resolved Hide resolved

samwilson force-pushed the filtering-ui-T218340 branch from a78304d to 8588ce6 Compare March 20, 2019 01:47

samwilson force-pushed the filtering-ui-T218340 branch from 8588ce6 to 8be071f Compare March 20, 2019 01:55

samwilson force-pushed the filtering-ui-T218340 branch from 8be071f to 84ecf0b Compare March 20, 2019 02:01

MusikAnimal mentioned this pull request Mar 20, 2019

Add support for categories without participants #217

Merged

samwilson force-pushed the filtering-ui-T218340 branch from 84ecf0b to 648eed6 Compare March 20, 2019 04:39

samwilson changed the title ~~Add filter warning messages~~ Reconfigure filter warning messages Mar 20, 2019

samwilson force-pushed the filtering-ui-T218340 branch 2 times, most recently from 12afb8d to 62b3baf Compare March 21, 2019 05:33

samwilson added Ready for review Ready for peer review WIP Work In Progress and removed Ready for review Ready for peer review labels Mar 21, 2019

samwilson force-pushed the filtering-ui-T218340 branch from 62b3baf to 9f411c2 Compare March 21, 2019 05:43

samwilson added Ready for review Ready for peer review and removed WIP Work In Progress labels Mar 21, 2019

MusikAnimal requested changes Mar 21, 2019

View reviewed changes

samwilson force-pushed the filtering-ui-T218340 branch 3 times, most recently from 6ecccb2 to 193711d Compare March 21, 2019 15:24

MusikAnimal requested changes Mar 21, 2019

View reviewed changes

samwilson force-pushed the filtering-ui-T218340 branch from 193711d to 4706291 Compare March 22, 2019 03:08

samwilson added WIP Work In Progress and removed Ready for review Ready for peer review labels Mar 22, 2019

samwilson force-pushed the filtering-ui-T218340 branch from 4706291 to 3b75081 Compare March 22, 2019 05:14

MusikAnimal approved these changes Mar 22, 2019

View reviewed changes

Reconfigure filter warning messages

66430ff

Set up three filter-message sections (only one of which is displayed) and modify the form-panel descriptions depending on the event state. Bug: T218340

samwilson force-pushed the filtering-ui-T218340 branch from 3b75081 to 66430ff Compare March 25, 2019 02:39

samwilson merged commit 6755541 into master Mar 25, 2019

samwilson deleted the filtering-ui-T218340 branch March 25, 2019 02:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reconfigure filter warning messages #234

Reconfigure filter warning messages #234

samwilson commented Mar 19, 2019 •

edited

MusikAnimal left a comment

MusikAnimal Mar 20, 2019 •

edited

samwilson Mar 20, 2019

MusikAnimal Mar 20, 2019

samwilson Mar 20, 2019

MusikAnimal commented Mar 20, 2019 •

edited

samwilson commented Mar 20, 2019

samwilson commented Mar 20, 2019

samwilson commented Mar 20, 2019

MusikAnimal left a comment •

edited

MusikAnimal Mar 21, 2019

samwilson Mar 21, 2019

MusikAnimal left a comment

samwilson commented Mar 22, 2019

MusikAnimal commented Mar 22, 2019

Reconfigure filter warning messages #234

Reconfigure filter warning messages #234

Conversation

samwilson commented Mar 19, 2019 • edited

MusikAnimal left a comment

Choose a reason for hiding this comment

MusikAnimal Mar 20, 2019 • edited

Choose a reason for hiding this comment

samwilson Mar 20, 2019

Choose a reason for hiding this comment

MusikAnimal Mar 20, 2019

Choose a reason for hiding this comment

samwilson Mar 20, 2019

Choose a reason for hiding this comment

MusikAnimal commented Mar 20, 2019 • edited

samwilson commented Mar 20, 2019

samwilson commented Mar 20, 2019

samwilson commented Mar 20, 2019

MusikAnimal left a comment • edited

Choose a reason for hiding this comment

MusikAnimal Mar 21, 2019

Choose a reason for hiding this comment

samwilson Mar 21, 2019

Choose a reason for hiding this comment

MusikAnimal left a comment

Choose a reason for hiding this comment

samwilson commented Mar 22, 2019

MusikAnimal commented Mar 22, 2019

samwilson commented Mar 19, 2019 •

edited

MusikAnimal Mar 20, 2019 •

edited

MusikAnimal commented Mar 20, 2019 •

edited

MusikAnimal left a comment •

edited