1869065 - Re-visit some of the element property names used in moz-message-bar

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, task)

Description

[Hanna Jones [:hjones]]

When creating moz-message-bar we were informally sticking to a convention of appending El as a suffix to property names that refer to DOM elements, e.g. in this code:

static queries = {
  actionsSlotEl: "slot[name=actions]",
  actionsEl: ".actions",
  closeButtonEl: "button.close",
};

The El suffix is really only necessary when we have a potential naming collision between another property and an element property - for example in moz-message-bar when we have a label reactive property but also want to query for and expose the label element. It could also be useful when we want to be explicit that a property refers to an element.

We filed Bug 1868827 to clarify when the suffix needs to be used. Some of the moz-message-bar properties are clear enough without the suffix, so we could probably just use something like:

static queries = {
  actionsSlot: "slot[name=actions]",
  actionsEl: ".actions",
  closeButton: "button.close",
};

Comment 1

[Sahil]

I would love to work on this, can you please assign it to me?

Comment 2

[Sahil]

(In reply to Sahil from comment #1)

I would love to work on this, can you please assign it to me?

I'm trying to locate the repository for this project but I'm unable to get it, can you please send the link of the repo?

Comment 3

[Ganna]

(In reply to Sahil from comment #2)

(In reply to Sahil from comment #1)

I would love to work on this, can you please assign it to me?

I'm trying to locate the repository for this project but I'm unable to get it, can you please send the link of the repo?

Here is a searchfox link to the file you need: https://searchfox.org/mozilla-central/source/toolkit/content/widgets/moz-message-bar/moz-message-bar.mjs#44

Comment 4

[Sahil]

I have made the required changes, how should I proceed? Sorry if I'm asking a bunch of basic questions, its my first time in this platform and i have never worked in a platform like this.

Comment 5

[Sahil]

:hjones (In reply to Hanna Jones [:hjones] from comment #0)

When creating moz-message-bar we were informally sticking to a convention of appending El as a suffix to property names that refer to DOM elements, e.g. in this code:

static queries = {
  actionsSlotEl: "slot[name=actions]",
  actionsEl: ".actions",
  closeButtonEl: "button.close",
};

The El suffix is really only necessary when we have a potential naming collision between another property and an element property - for example in moz-message-bar when we have a label reactive property but also want to query for and expose the label element. It could also be useful when we want to be explicit that a property refers to an element.

We filed Bug 1868827 to clarify when the suffix needs to be used. Some of the moz-message-bar properties are clear enough without the suffix, so we could probably just use something like:

static queries = {
  actionsSlot: "slot[name=actions]",
  actionsEl: ".actions",
  closeButton: "button.close",
};

I have the modified file with me, please assign this to me , so that I can send the patch file

Comment 6

[Ganna]

(In reply to Sahil from comment #4)

I have made the required changes, how should I proceed? Sorry if I'm asking a bunch of basic questions, its my first time in this platform and i have never worked in a platform like this.

You don't have to be assigned to the bug to submit the patch. Here is a link to the contributor's guide:
https://firefox-source-docs.mozilla.org/contributing/contribution_quickref.html
Feel free to ask any additional questions.

Comment 7

[Hanna Jones [:hjones]]

hey :sahil thanks for your interest in working on this! If you have the change ready to go locally you can submit a patch for review following these instructions: https://firefox-source-docs.mozilla.org/testing/marionette/Patches.html

If you have any more specific questions you can request information from me using the form below (I'll get a reminder that way, I don't always get notified about comments). Alternatively you can also reach out to me in the reusable component channel on Matrix.

Comment 8

[gravyant]

Hi, I'd like to take on this bug and will submit a patch soon.

Comment 9

[gravyant]

Attachment: Bug 1869065 - Re-visit some of the element property names used in moz-message-bar r?hjones

Comment 10

[gravyant]

I submitted a patch renaming the property keys. Wouldn't I also need to change the code that imports MozMessageBar and tries access those properties?

Comment 11

[Hanna Jones [:hjones]]

hey :gravyant thanks for picking this up and submitting a patch! You're right that we need to change a handful of places in the code that accesses those MozMessageBar properties - I should have called that out in the original comment to give a better sense of scope. I left a comment with some next steps on your patch, feel free to respond there or request info from me here if you have any additional questions.

Comment 12

[gravyant]

Attachment: Bug 1869065 - Re-visit some of the element property names used in moz-message-bar r?hjones

Comment 13

[gravyant]

Hi :hjones, I had issues with version control so I submitted a new patch. It should include the changes that you mentioned. I also went ahead and included changes for the two shopping related files, as well as the browser_jsterm_autocomplete_getters_cancel.js file. Are there any specific tests that you would like for me to run?

Comment 14

[Hanna Jones [:hjones]]

hey :gravyant I saw the patch before I saw the NI here and addressed how I think you can unify the two patches in a comment there. Feel free to request info again if that doesn't work!

(In reply to gravyant from comment #13)

Hi :hjones, I had issues with version control so I submitted a new patch. It should include the changes that you mentioned. I also went ahead and included changes for the two shopping related files, as well as the browser_jsterm_autocomplete_getters_cancel.js file. Are there any specific tests that you would like for me to run?

Ah I think there may have been a communication breakdown here - I was trying to say you'll need to update all the closeButtonEl references found by this query EXCEPT for those in shopping and devtools. Shopping and devtools aren't using moz-message-bar, the presence of a closeButtonEl variable in those cases is just a coincidence. After you make the changes I would recommend running any moz-message-bar tests and any tests you make changes to. Please let me know if you have any further questions!

Comment 15

[gravyant]

Attachment: Bug 1869065 - Re-visit some of the element property names used in moz-message-bar. r?hjones

Comment 16

[gravyant]

Hi :hjones, I couldn't get the commands that you suggested to work, so I revised my patch and abandoned the other two revisions. Does that work?

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2d10c611fd6d
Re-visit some of the element property names used in moz-message-bar. r=hjones,reusable-components-reviewers,omc-reviewers,aminomancer

Comment 18

[Aron Cseh [:acseh]]

https://hg.mozilla.org/mozilla-central/rev/2d10c611fd6d