1525992 - Rename TextProperty to Declaration

Status: NEW

(DevTools :: Inspector: Rules, enhancement, P3)

Description

[Gabriel Luong [:gl] (ΦωΦ)]

Instead of calling a property + value a TextProperty, we moved to using the correct syntax "Declaration" in the new rules view. We should rename text-property.js to declaration.js and rename all instances of TextProperty, text property, and the variants to declaration.

https://developer.mozilla.org/en-US/docs/Web/CSS/Syntax

Comment 1

[Uday Mewada]

Attachment: Bug 1525992 - Ranamed TextProperty to Daclaration. r=gl

Comment 2

[Uday Mewada]

sir submitted a patch, have a look.

Comment 3

[Uday Mewada]

Attachment: Bug 1525992 - added the renamed file r=gl

Depends on D63999

Comment 4

[Uday Mewada]

should I make changes in these files also https://searchfox.org/mozilla-central/search?q=TextProperty&case=false&regexp=false&path=

Comment 5

[Gabriel Luong [:gl] (ΦωΦ)]

(In reply to Uday Mewada from comment #4)

should I make changes in these files also https://searchfox.org/mozilla-central/search?q=TextProperty&case=false&regexp=false&path=

Right, so I would expect every instance where we call TextProperty to be refactored to Declaration. We also have instances where we use textProp. We want to follow the CSS syntax (https://developer.mozilla.org/en-US/docs/Web/CSS/Syntax) where a Declaration is inclusive of the property name and value pair. We should also be mindful of instances where we call property, and figure out if we mean Declaration, Property Name or Property Value and adjust the naming to be specific to the actual value.

Since this refactoring will touch a lot of files and may require further context, I don't think I would recommend this as a good-first-bug. The process will be take considerable amount of time and effort to make sure you don't break anything from the refactoring and ensure correctness.

Comment 6

[Uday Mewada]

sir, I changed TextProperty to Declaration.
shouls I also change the TextPropertyEditor to DeclarationEditor present here https://searchfox.org/mozilla-central/source/devtools/client/inspector/rules/views/text-property-editor.js#94.

Comment 7

[Gabriel Luong [:gl] (ΦωΦ)]

(In reply to Uday Mewada from comment #6)

sir, I changed TextProperty to Declaration.
shouls I also change the TextPropertyEditor to DeclarationEditor present here https://searchfox.org/mozilla-central/source/devtools/client/inspector/rules/views/text-property-editor.js#94.

I think we should leave that to be a separate bug to make things easy to review.

Comment 8

[Uday Mewada]

what about textProp instance.

Comment 9

[Gabriel Luong [:gl] (ΦωΦ)]

(In reply to Uday Mewada from comment #8)

what about textProp instance.

I want to say yes, but that's why this bug will be an intense effort. You would also need to figure out if textProp is being used correctly. You could do this in a separate commit, but I wouldn't want to land anything until we address all the TextProperty variants.

Comment 10

[Uday Mewada]

Attachment: Bug 1525992 - Renamed TextProperty to Declaration and text-property.js to declaration.js r=gl

Comment 11

[Uday Mewada]

Attachment: Bug 1525992-Renamed TextProperty to Declaration and text-property.js to declaration.js r=gl

Comment 12

[Razvan Caliman [:rcaliman]]

Hi all,

I want to chime in here and kindly ask not to handle this bug at this time. The TextProperty terminology is still used widely in the codebase, particularly in tests.

Also, we have two versions of the Rules panel, one legacy and still active, and one React/Redux-based not yet enabled. The legacy version makes extensive use of TextProperty. The new one transitioned to Declaration, but still relies on code shared with the legacy version (models/element-style.js models/text-property-editor.js, etc.)

I'm reviewing a big patch in bug 1596093 for updating just a few tests to a getTextProperty() helper (to be renamed later to getDeclaration()) and the amount of rule.textProp instances in tests is still very large. Many tests will need to be updated so the terminology makes sense.

A clearer course of action would be:

• postpone working on this task to rename TextProperty to Declaration
• continue work on the new Rules panel (which already uses the "declaration" terminology)
• fix tests to ensure they work with new Rules panel
• remove the legacy Rules panel (and with it all the "TextProperty' naming)
• update the leftover TextProperty instances to Declaration in tests and other files.

So, for now, please do not address this task.

Uday, to avoid situations like these in the future, it helps to announce that you want to work on a bug before starting on it. Team members will be happy to provide guidance. For some bugs, like this one, there is more nuance involved than what's in the bug description. As Gabriel mentioned in comment #5, this is not really a good-first-bug to start on.

Comment 13

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.