Skip to content

Refactor dropdown scroll to menu#11710

Merged
andreslucena merged 3 commits intodevelopfrom
chore/dropdown-scroll-to-menu
Oct 4, 2023
Merged

Refactor dropdown scroll to menu#11710
andreslucena merged 3 commits intodevelopfrom
chore/dropdown-scroll-to-menu

Conversation

@fblupi
Copy link
Copy Markdown
Member

@fblupi fblupi commented Oct 3, 2023

🎩 What? Why?

As @Crashillo commented here after the PR was merged, I have opened a new one following the tips he left.

📌 Related Issues

Testing

From a mobile phone:

  1. Go to a conference or participatory space.
  2. Click on the "Jump to" button.
  3. See the page scrolls down so the menu is shown.

📷 Screenshots

Screen.Recording.2023-09-29.at.14.01.51.mov

♥️ Thank you!

@fblupi fblupi added javascript Pull requests that update Javascript code type: internal PRs that aren't necessary to add to the CHANGELOG for implementers project: redesign Barcelona City Council contract labels Oct 3, 2023
@fblupi fblupi requested review from a team and Crashillo October 3, 2023 07:45
Comment thread decidim-core/app/packs/src/decidim/redesigned_a11y.js Outdated
Crashillo
Crashillo previously approved these changes Oct 3, 2023
Copy link
Copy Markdown
Contributor

@Crashillo Crashillo left a comment

Choose a reason for hiding this comment

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

Yeea! much better, simpler, cleaner... :) ...............This is the way

Co-authored-by: Andrés Pereira de Lucena <andreslucena@users.noreply.github.com>
Copy link
Copy Markdown
Contributor

@alecslupu alecslupu left a comment

Choose a reason for hiding this comment

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

LGTM

@alecslupu alecslupu added this to the 0.28.0 milestone Oct 3, 2023
@alecslupu
Copy link
Copy Markdown
Contributor

@andreslucena Since you also reviewed this PR i am waiting your input as well before merging.

Copy link
Copy Markdown
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.

It works, but I'm not happy with the current approach on this file regarding the number of characters by each line. In this particular line we're on 247 characters.

I'm not saying that we need to go the oldschool way of 80 characters, but it really makes difficult to understand whats going on here.

As I see that there are others places in this file where we're doing the same, I'll merge this one and make a refactor of this file with what I have in mind, as that'll be faster.

Thanks for the PR with the fix and taking care of this issue guys!

@andreslucena andreslucena merged commit b22f8a3 into develop Oct 4, 2023
@andreslucena andreslucena deleted the chore/dropdown-scroll-to-menu branch October 4, 2023 08:34
entantoencuanto added a commit that referenced this pull request Oct 6, 2023
* feature/renaming-redesign:
  Use default current participatory space scope as root on scopes_select_tag called from bulk actions
  restore the imports thing (compilation fails for initiatives)
  change testing color: dequelabs/axe-core#3513 (comment)
  Fix admin redesign module (#11648)
  Apply flash styles to Announcements (#11708)
  Refactor oneliners of redesigned_a11y.js (#11713)
  Redesign: fix votings admin module issues (#11704)
  Add meta robots noindex to search and profile (#10120)
  Refactor dropdown scroll to menu (#11710)
  Remove unused partial
  Remove REDESIGN_PENDING obsolete comand
  Redesign: fix assembly members sidebar menu (#11699)
  Redesign: fix conferences admin module issues (#11703)
  Redesign: fix assemblies admin module issues (#11702)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

javascript Pull requests that update Javascript code project: redesign Barcelona City Council contract type: internal PRs that aren't necessary to add to the CHANGELOG for implementers

Projects

No open projects
Archived in project

Development

Successfully merging this pull request may close these issues.

4 participants