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

feat: bring embed builder field manipulation in line with underlying array functionality #3761

Merged
merged 8 commits into from
Feb 23, 2020

Conversation

almostSouji
Copy link
Member

@almostSouji almostSouji commented Feb 2, 2020

Please describe the changes this PR makes and why it should be merged:

  • remove MessageEmbed#spliceField
  • add MessageEmbed#spliceFields
  • allow multiple fields to be replaced/inserted
  • update typings accordingly
  • add MessageEmbed#addFields
  • remove MessageEmbed#addField
  • support multi-field input as Array and rest parameters
  • bump node to 11 for Array#flat

The applied approach brings the embed utility methods more in line with the underlying actions done on the raw field array (push, splice) while (hopefully) cleanly handling both rest parameters as well as array passing.

While it does remove some convenience, namely .addField("foo", "bar") we discussed that keeping both #addField and #addFields simultaneously should not be the way to go, adding more noise to the interface than necessary (see the earlier deprecation of #addFile in favor of #addFiles)

  • If there are really strong opinions of library maintainers i can of course revert the #addField removal
  • the change to splice however adds utility in freely modifying embed fields while not removing any functionality that is currently in and should definitely be considered

Example

const e = new Discord.MessageEmbed()
			.addFields(
				{ name: 1, value: 1 },
				{ name: 2, value: 2 },
				{ name: 3, value: 3 },
				{ name: 4, value: 4 }
			)
			.spliceFields(2, 1,
				{ name: 'foo', value: 'bar', inline: true },
				{ name: 'foo2', value: 'bar2', inline: true });
message.channel.send(e);

ℹ Both methods also support the fields to be provided as an Array
⚠Due to the use of Array#flat this will require a min node bump to 11

TODO:

  • refactor checkField to accept multiples and be named normalizeFields
  • add MessageEmbed#addFields
  • remove MessageEmbed#addFields
  • bump min node
  • test typings

Status

  • Code changes have been tested against the Discord API, or there are no code changes
  • I know how to update typings and have done so, or typings don't need updating

Semantic versioning classification:

  • This PR changes the library's interface (methods or parameters added)
    • This PR includes breaking changes (methods removed or renamed, parameters moved or removed)
  • This PR only includes non-code changes, like changes to documentation, README, etc.

* remove MessageEmbed#spliceField
* add MessageEmbed#spliceFields
* to behave more like Array#splice
* and allow multiple fields to be replaced/inserted
* update typings accordingly
@almostSouji almostSouji changed the title feat: splice multiple MessageEmbed fields [WIP] feat: splice multiple MessageEmbed fields Feb 2, 2020
@almostSouji almostSouji changed the title [WIP] feat: splice multiple MessageEmbed fields [WIP] feat: splice multiple MessageEmbed fields, replace #addField with #addFields Feb 2, 2020
@almostSouji almostSouji force-pushed the messageembed-splice branch 2 times, most recently from a66f2f7 to eb6c39e Compare February 2, 2020 17:10
@PyroTechniac
Copy link
Contributor

This looks great! Can you also bump the node version in the package.json engines field?

@almostSouji almostSouji changed the title [WIP] feat: splice multiple MessageEmbed fields, replace #addField with #addFields feat: bring embed builder field manipulation in line with underlying array functionality Feb 2, 2020
@PyroTechniac
Copy link
Contributor

Also, looking into it, can't a generator function be introduced to flatten arrays without having to bump to a different node version?

@iCrawl
Copy link
Member

iCrawl commented Feb 3, 2020

Also, looking into it, can't a generator function be introduced to flatten arrays without having to bump to a different node version?

This won't be necessary as we are probably going to target 12 anyway, latest LTS

@almostSouji
Copy link
Member Author

almostSouji commented Feb 20, 2020

As Pyro pointed out on the server:
This needs to also handle addBlankField to some capacity.

  • remove it, since it's really just adding a field with a zero width space
  • keep it as is and use .addFields in the background
  • keep it and adapt it to addBlankFields taking a number

I'm personally in favor of 1, as from experience tempering with embed layout using blank fields never yields the desired result in the first place, even more so after the rather recent embed update in which discord restyled embeds.

Edit:
As of feedback from crawl and space: removed it in the following commit

@SpaceEEC SpaceEEC added this to the 12.0.0 milestone Feb 22, 2020
@SpaceEEC SpaceEEC merged commit b727f6c into discordjs:master Feb 23, 2020
samsamson33 pushed a commit to samsamson33/discord.js that referenced this pull request Feb 27, 2020
…array functionality (discordjs#3761)

* feat: splice multiple fields

* remove MessageEmbed#spliceField
* add MessageEmbed#spliceFields
* to behave more like Array#splice
* and allow multiple fields to be replaced/inserted
* update typings accordingly

* refactor: rename check to normalize

* check suggests boolean return type

* feat: allow spread args or array as field input

* rewrite: replace addField in favor of addFields

* typings: account for changes

* chore: bump min node to 11.0.0

* for Array#flat

* fix: bump min-node in package engines field

* remove addBlankField
samsamson33 added a commit to samsamson33/discord.js that referenced this pull request Feb 27, 2020
@almostSouji almostSouji deleted the messageembed-splice branch March 1, 2020 02:43
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.

5 participants