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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove xlarge-* references from admin forms #11712

Merged
merged 1 commit into from
Oct 16, 2023
Merged

Remove xlarge-* references from admin forms #11712

merged 1 commit into from
Oct 16, 2023

Conversation

alecslupu
Copy link
Contributor

馃帺 What? Why?

This PR attempts to clean up the HTML markup in admin by removing useless class xlarge-*

Testing

  1. Login as admin go to admin panel
  2. From the files section try to find the changed forms
  3. See there are no changes regarding UI

鈾ワ笍 Thank you!

@alecslupu alecslupu added the type: internal PRs that aren't necessary to add to the CHANGELOG for implementers label Oct 3, 2023
@alecslupu alecslupu added this to the 0.28.0 milestone Oct 3, 2023
andreslucena
andreslucena reviewed Oct 13, 2023
View reviewed changes
Copy link
Member

@andreslucena andreslucena left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was just skiming this PR and I don't like the approach as we're mixing multiple problems in the same PR:

I'd prefer to handle each of these issues in different PRs, as if we're mixing problems/solutions, then these PRs become huge and messy. These already are medium/big but if we keep adding things then the review is just too much work.

I believe that if we have PRs like #11707 (that goes to a linter configuration change) then that should be easy-peasy to review/merge and to not bring noise to the rest of PRs.

What do you think @alecslupu?

@alecslupu
Copy link
Contributor Author

I was just skiming this PR and I don't like the approach as we're mixing multiple problems in the same PR:

* the dead CSS classes (explained at [Old CSS classes still in the views #11752](https://github.com/decidim/decidim/issues/11752))

* the problem with legacy HTML markup structure

* some indentation changes (related with [Apply new rubocop rules on erb - Argument identation #11707](https://github.com/decidim/decidim/pull/11707))

I'd prefer to handle each of these issues in different PRs, as if we're mixing problems/solutions, then these PRs become huge and messy. These already are medium/big but if we keep adding things then the review is just too much work.

I believe that if we have PRs like #11707 (that goes to a linter configuration change) then that should be easy-peasy to review/merge and to not bring noise to the rest of PRs.

What do you think @alecslupu?

@andreslucena you are right. I have refactored this PR. Since this is draft, i assume you have not really started to review it :D

i have added the ERB syntax as well for deprecated classes.

@alecslupu alecslupu marked this pull request as ready for review October 13, 2023 17:58
@alecslupu alecslupu mentioned this pull request Oct 13, 2023
Closed
andreslucena
andreslucena approved these changes Oct 14, 2023
View reviewed changes
Copy link
Member

@andreslucena andreslucena left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cheff kiss gif

@andreslucena andreslucena mentioned this pull request Oct 15, 2023
Merged
@alecslupu
Copy link
Contributor Author

Merging as per @andreslucena 's approval.

@alecslupu alecslupu merged commit 6e08506 into develop Oct 16, 2023
104 checks passed
@alecslupu alecslupu deleted the chore/remove-admin-xlarge branch October 16, 2023 06:18
andreslucena pushed a commit that referenced this pull request Oct 16, 2023
@alecslupu @andreslucena
Remove xlarge-* references from admin forms (#11712)
67b2468
entantoencuanto added a commit that referenced this pull request Oct 17, 2023
@entantoencuanto
Merge branch 'develop' into feature/redesign-staging
7ceeba2 
* develop: (30 commits)
  Add `process-content` to erb-lint's deprecated classes (#11762)
  Add possibility of overriding the tailwind.config.js (#11763)
  Ask old password when changing email or password (#11737)
  Add Projects (Budgets) to filtered search (#11740)
  Fix missing results on Geocoded when search without diacritics (#11761)
  Add robots.txt instructions (#11693)
  Add missing activerecord budget locales for search (#11766)
  Improve design of Admin's Sidebar pages (#11759)
  Show small static map on admin's meetings index with big screens  (#11715)
  Remove "Manage" button when there's a Sidebar (#11717)
  Fix admin breadcrumb in Process (#11757)
  Apply new rubocop rules on erb - Layout/MultilineMethodCallIndentation (#11756)
  Remove xlarge-* references from admin forms (#11712)
  Apply new rubocop rules on erb - Argument identation (#11707)
  Update HERE API autocomplete (#11507)
  Admin redesign proposal issues (#11668)
  Redesign: responsive links on cards (#11538)
  Refactor CI pipelines (#11196)
  Update postcss and graphql to latest versions (#11733)
  Fix develop pipeline (#11750)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andreslucena andreslucena andreslucena approved these changes

Assignees
No one assigned
Labels
type: internal PRs that aren't necessary to add to the CHANGELOG for implementers
Projects
Maintainers' backlog
Archived in project
Milestone
0.28.0
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@alecslupu @andreslucena