Add site names on sites-list error notices #483
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
johnHackworth
added
Sites
[Status] Needs Review
johnHackworth
force-pushed
the
update/sites-list-add-site-names-on-error-notices
branch
3 times, most recently
from
4185e66 to
de1c371
Compare
| numberOfSites: logs.length | ||
| } | ||
| }; | ||
| var sampleLog = logs[ 0 ], |
johnHackworth
force-pushed
the
update/sites-list-add-site-names-on-error-notices
branch
from
de1c371 to
9de2f95
Compare
|
I think this is a good start. Especially when we move to the pill notifications we need to consider to revisit this. I think this is a good stop gap solution. |
| args: { | ||
| siteName: sampleLog.site.title, | ||
| siteNames: sites.join( ', ' ), | ||
| numberOfSites: logs.length |
There was a problem hiding this comment.
I think this might need to get changed to sites.length. Since the number of sites might be different from the log. ( I am not 100% sure about )
|
One minor change but otherwise this looks good to go 👍 |
|
This is definitely an improvement. Can we have a period at the end of the sentence? |
|
Just updated the code to include periods at the end of sentences and the suggestion I made. |
enejb
added
[Status] Ready to Merge
and removed
[Status] Needs Review
johnHackworth
force-pushed
the
update/sites-list-add-site-names-on-error-notices
branch
from
cf896e6 to
a1801bc
Compare
johnHackworth
added a commit
that referenced
this pull request
Nov 25, 2015
…mes-on-error-notices Add site names on sites-list error notices
johnHackworth
deleted the
update/sites-list-add-site-names-on-error-notices
branch
lancewillett
removed
the
[Status] Needs Design Review
This was referenced Jun 8, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Until now, each time you got an error message related with your site list that affected more than one site, you only could see that the error were affecting N sites, but not which sites those were.
This PR adds the name of the sites to the error message:
How to test