jenkins icon indicating copy to clipboard operation
jenkins copied to clipboard

Remove '.tr' from advanced-button

Open janfaracik opened this issue 1 year ago • 2 comments

Would be good to get some discussion on this.

Shifting Jenkins away from depending on classnames for JS selectors would be a beneficial move. Classnames tend to be unwieldy and can complicate refactoring and comprehension. This PR tackles one such area by replacing the class selector (.TR) with a new attribute:data-type='jenkins-advanced-button-group'. This makes the code easier to search and keeps the class attribute just for styling.

Testing done

  • Advanced button works as before, no usages found on GitHub

Proposed changelog entries

  • N/A

Proposed upgrade guidelines

N/A

### Submitter checklist
- [ ] The Jira issue, if it exists, is well-described.
- [ ] The changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developers, depending on the change) and are in the imperative mood (see [examples](https://github.com/jenkins-infra/jenkins.io/blob/master/content/_data/changelogs/weekly.yml)). Fill in the **Proposed upgrade guidelines** section only if there are breaking changes or changes that may require extra steps from users during upgrade.
- [ ] There is automated testing or an explanation as to why this change has no tests.
- [ ] New public classes, fields, and methods are annotated with `@Restricted` or have `@since TODO` Javadocs, as appropriate.
- [ ] New deprecations are annotated with `@Deprecated(since = "TODO")` or `@Deprecated(forRemoval = true, since = "TODO")`, if applicable.
- [ ] New or substantially changed JavaScript is not defined inline and does not call `eval` to ease future introduction of Content Security Policy (CSP) directives (see [documentation](https://www.jenkins.io/doc/developer/security/csp/)).
- [ ] For dependency updates, there are links to external changelogs and, if possible, full differentials.
- [ ] For new APIs and extension points, there is a link to at least one consumer.

Desired reviewers

@jenkinsci/sig-ux

Before the changes are marked as ready-for-merge:

### Maintainer checklist
- [ ] There are at least two (2) approvals for the pull request and no outstanding requests for change.
- [ ] Conversations in the pull request are over, or it is explicit that a reviewer is not blocking the change.
- [ ] Changelog entries in the pull request title and/or **Proposed changelog entries** are accurate, human-readable, and in the imperative mood.
- [ ] Proper changelog labels are set so that the changelog can be generated automatically.
- [ ] If the change needs additional upgrade steps from users, the `upgrade-guide-needed` label is set and there is a **Proposed upgrade guidelines** section in the pull request title (see [example](https://github.com/jenkinsci/jenkins/pull/4387)).
- [ ] If it would make sense to backport the change to LTS, a Jira issue must exist, be a _Bug_ or _Improvement_, and be labeled as `lts-candidate` to be considered (see [query](https://issues.jenkins.io/issues/?filter=12146)).

janfaracik avatar Aug 17 '23 19:08 janfaracik

Any references to this approach you could provide?

timja avatar Aug 18 '23 08:08 timja

Could you go into more detail regarding the motivation for this? I don't understand it. Some first thoughts for this:

Length

Classnames tend to be unwieldy and can complicate refactoring and comprehension.

Seems largely due to BEM which IIRC you (co)introduced to Jenkins and seems to have enough benefits for us to use despite the verbosity. Are we turning our back on that?

jenkins-advanced-button-group isn't exactly un-unwieldy either.

Also, aren't long names easier to handle with refactoring, as they're easy to unambiguously grep for?

Dataset use

This approach introduces a new thing, data-type, which is like a class but not really, which needs to be understood first because it can be used. Class attributes are common.

data-* attributes bring with them the problem that the camel casing in JS dataset makes them more difficult to grep for (not unlike JEXL/Groovy property conventions).

keeps the class attribute just for styling

Is that an actual convention or is that just your desired end state? While there's a lot of styling based on classes in CSS, it's not like they're unused elsewhere (e.g. attaching behaviours in JS).

Cost vs. benefit

Overall the impression that I get is that this might be a convention that can be done in a new project when there's no code written yet. With Jenkins, we're unlikely to ever finish the migration that would be needed in the project to end up in a consistent state, and it seems to have very limited benefits.

daniel-beck avatar Aug 24 '23 20:08 daniel-beck