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.

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

Already on GitHub? Sign in to your account

Use "New section" as placeholder, empty title as the hidden/builtin title #15581

Merged
merged 9 commits into from
May 16, 2024

Conversation

oliverguenther
Copy link
Member

cbliard
cbliard previously approved these changes May 16, 2024
@cbliard
Copy link
Member

cbliard commented May 16, 2024

@oliverguenther
The placeholder text is "New section" and yet when creating the section without having entered any name, the section title is "Untitled"
I would have expected "New section" as section title as it is what the placeholder text was.
Is it expected?

@oliverguenther oliverguenther force-pushed the fix/new-section-title branch 2 times, most recently from 3a9c3e8 to f0a3ddd Compare May 16, 2024 09:38
@oliverguenther
Copy link
Member Author

@oliverguenther The placeholder text is "New section" and yet when creating the section without having entered any name, the section title is "Untitled" I would have expected "New section" as section title as it is what the placeholder text was. Is it expected?

Good catch. Indeed this should be changed to make it consistent

@oliverguenther oliverguenther force-pushed the fix/new-section-title branch from f0a3ddd to 134b567 Compare May 16, 2024 10:38
Copy link

1 Warning
⚠️ This PR has migration-related changes on a release branch. Ping @opf/operations

Generated by 🚫 Danger

@oliverguenther oliverguenther changed the title Use "New section" as placeholder Use "New section" as placeholder, empty title as the hidden/builtin title May 16, 2024
@oliverguenther oliverguenther force-pushed the fix/new-section-title branch from f61ede5 to 0672230 Compare May 16, 2024 12:07
@oliverguenther oliverguenther requested a review from jjabari-op May 16, 2024 12:07
@oliverguenther oliverguenther dismissed cbliard’s stale review May 16, 2024 12:08

Additional changes since your review

Copy link
Member

@cbliard cbliard left a comment

Choose a reason for hiding this comment

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

It does not seem to work as expected: it should not allow creating a section without a title. Yet it does.

Also there are some strange things: in MeetingSectionsController, the meeting section is initially created with title "New section", but this one is not shown in the returned HTML.

It's because in modules/meeting/app/forms/meeting_sections/title.rb, the title is not used if it is the default title. Why not just use the given value?

When saving, it then accepts the empty value, contrary to the text comment in create contract.

The check in create contract is not run. To ensure the title is present this check should also be done in the update contract of meeting section.

Comment on lines 34 to 36
# We allow an empty title to mark an untitled/implicit section
# but users should not be able to create it with an empty title
validates :title, presence: true
Copy link
Member

Choose a reason for hiding this comment

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

This check should be replicated in the UpdateContract to prevent users from emptying the section title.

Copy link
Member

@cbliard cbliard left a comment

Choose a reason for hiding this comment

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

The previous issue is fixed: when trying to create a section with a blank title, it now displays "Title can't be blank"

I saw 3 other issues, one of which is intentional:

  • "+ Add" button turning blank
  • section existing with empty title when reloading the page
  • incorrect place holder text when editing an existing section having an empty title

"+ Add" button turning blank

Not sure why, but when adding a new section, a new work package or a new agenda item, the green "+ Add" button at the bottom of the page is turning blank.

image

Is it because it is in a disabled state?


Section existing with empty title when reloading the page

steps:

  1. click "+ Add" > click "Section"
    => a section is added, prompting for a title
  2. reload the page
    => the section is there, with an empty title

This is the one Oliver explained me that it is kinda intentional, as the section is already created when it is prompting for a title.
I'm just putting it here for acknowledgement.


Incorrect place holder text when editing an existing section having an empty title

Steps:

  1. Add a section with a proper name
  2. Edit the first section name (the one being displayed as "Untitled section")
  • Actual: the placeholder is "Title"
  • Expected: the placeholder should be "New section"

image

@jjabari-op
Copy link
Collaborator

@cbliard thank you for your remarks:

Towards No 1:

Indeed, the disabled state looks not correctly styled - I try to figure out what is causing this

Towards No 2:

Yes, this is intentional

Towards No 3:

Fixed

Copy link
Member

@cbliard cbliard left a comment

Choose a reason for hiding this comment

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

Sounds good now

@jjabari-op
Copy link
Collaborator

jjabari-op commented May 16, 2024

@cbliard towards No 1 again: var(--button-primary-bgColor-disabled) seems not to be defined anymore according to my chrome inspector:
Bildschirmfoto 2024-05-16 um 17 14 09

I create a bug for that

@cbliard cbliard merged commit e263c63 into release/14.1 May 16, 2024
11 checks passed
@cbliard cbliard deleted the fix/new-section-title branch May 16, 2024 15:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants