Added selected connection count#1020
Conversation
|
I have few ESLint errors and I already tried to resolve by installing ESLint and Prettier ESLint extension but still nothing works so If anyone can take a look then it will be appreciated. |
|
@roshniahuja I've pushed 22a5031 which fixes the linting errors. Getting the lints set up in your IDE can be luck as much as anything else. If you use VS Code, I can share my extensions with you if you reach out via Slack. |
peterwilsoncc
left a comment
peterwilsoncc
left a comment
There was a problem hiding this comment.
A couple of inline notes about consistency with layout/spacing within the brackets.
I'm wondering if it would be better to use the string Selected connections (%d) to ensure the strings are translatable in to all language formats.
The HTML could become:
<header class="with-selected">
<span class="selected-connections-text"><?php esc_html_e( 'Selected connections', 'distributor' ); ?></span>
<button class="button button-link selectno-connections unavailable"><?php esc_html_e( 'Clear', 'distributor' ); ?></button>
</header>And the text in selected-connections-text replaced in full via the JavaScript. @ravinderk, do you have an opinion on what would be best?
roshniahuja
added
needs:code-review
peterwilsoncc
left a comment
peterwilsoncc
left a comment
There was a problem hiding this comment.
This looks great, thank you!
One minor note about translator comments for _n but otherwise it looks good.
assets/js/push.js
Outdated
| delete selectedConnections[ type + id ]; | ||
|
|
||
| selectedText = sprintf( | ||
| /* translators: 1) Selected connection content singular name. 2) Selected connection content plural name. */ |
There was a problem hiding this comment.
| /* translators: 1) Selected connection content singular name. 2) Selected connection content plural name. */ | |
| /* translators: %d: Number of selected connections. */ |
For _n translator notes are a little odd, the comment applies to both strings as the placeholders need to be identical.
assets/js/push.js
Outdated
|
|
||
| const element = event.currentTarget.cloneNode( true ); | ||
| selectedText = sprintf( | ||
| /* translators: 1) Selected connection content singular name. 2) Selected connection content plural name. */ |
There was a problem hiding this comment.
| /* translators: 1) Selected connection content singular name. 2) Selected connection content plural name. */ | |
| /* translators: %d: Number of selected connections. */ |
There was a problem hiding this comment.
Comment Updated.
assets/js/push.js
Outdated
| element.appendChild( removeLink ); | ||
| element.classList = 'added-connection'; | ||
| selectedText = sprintf( | ||
| /* translators: 1) Selected connection content singular name. 2) Selected connection content plural name. */ |
There was a problem hiding this comment.
| /* translators: 1) Selected connection content singular name. 2) Selected connection content plural name. */ | |
| /* translators: %d: Number of selected connections. */ |
assets/js/push.js
Outdated
|
|
||
| delete selectedConnections[ type + id ]; | ||
| selectedText = sprintf( | ||
| /* translators: 1) Selected connection content singular name. 2) Selected connection content plural name. */ |
There was a problem hiding this comment.
| /* translators: 1) Selected connection content singular name. 2) Selected connection content plural name. */ | |
| /* translators: %d: Number of selected connections. */ |
assets/js/push.js
Outdated
| delete selectedConnections[ type + id ]; | ||
|
|
||
| selectedText = sprintf( | ||
| /* translators: 1) Selected connection content singular name. 2) Selected connection content plural name. */ |
There was a problem hiding this comment.
| /* translators: 1) Selected connection content singular name. 2) Selected connection content plural name. */ | |
| /* translators: %d: Number of selected connections. */ |
peterwilsoncc
left a comment
peterwilsoncc
left a comment
There was a problem hiding this comment.
Looks good to me, thank you!
4 participants
Fixes for issue #592
Description of the Change
Issue mentioned URL - #592
Added selected connection count
Closes
How to test the Change
To test this feature - Need to select connection from dropdown and it will increase count in Selected connection
Changelog Entry
Credits
Props @roshniahuja
Checklist: