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

feat: add dialog title, header and footer renderers #3623

Merged
merged 29 commits into from
Apr 4, 2022

Conversation

DiegoCardoso
Copy link
Contributor

@DiegoCardoso DiegoCardoso commented Mar 30, 2022

Description

Adds new features to dialog:

  • New header and footer parts
  • Title property (headerTitle)
  • Two new renderers (headerRenderer and footerRenderer)

Fixes #1513

Type of change

  • Bugfix
  • Feature

Additional for Feature type of change

  • Enhancement / new feature was discussed in a corresponding GitHub issue and Acceptance Criteria were created.

packages/dialog/src/vaadin-dialog-footer.js Outdated Show resolved Hide resolved
packages/dialog/src/vaadin-dialog-header.js Outdated Show resolved Hide resolved
packages/dialog/src/vaadin-dialog.js Outdated Show resolved Hide resolved
packages/dialog/src/vaadin-dialog.js Outdated Show resolved Hide resolved
packages/dialog/src/vaadin-dialog.js Outdated Show resolved Hide resolved
packages/dialog/src/vaadin-dialog.js Outdated Show resolved Hide resolved
packages/dialog/src/vaadin-dialog.js Outdated Show resolved Hide resolved
@DiegoCardoso
Copy link
Contributor Author

DiegoCardoso commented Mar 31, 2022

The PR is almost ready.

Missing:

  • Visual tests
  • JSdoc
  • Type definition

packages/dialog/src/vaadin-dialog.js Outdated Show resolved Hide resolved
packages/dialog/src/vaadin-dialog.js Outdated Show resolved Hide resolved
packages/dialog/src/vaadin-dialog.js Outdated Show resolved Hide resolved
packages/dialog/src/vaadin-dialog.js Outdated Show resolved Hide resolved
packages/dialog/src/vaadin-dialog.js Outdated Show resolved Hide resolved
packages/dialog/test/header-footer.test.js Outdated Show resolved Hide resolved
packages/dialog/test/header-footer.test.js Outdated Show resolved Hide resolved
packages/dialog/test/header-footer.test.js Outdated Show resolved Hide resolved
packages/dialog/theme/material/vaadin-dialog-styles.js Outdated Show resolved Hide resolved
packages/dialog/theme/material/vaadin-dialog-styles.js Outdated Show resolved Hide resolved
@web-padawan web-padawan changed the title feat: Dialog header and footer feat: add dialog title, header and footer renderers Apr 1, 2022
Copy link
Contributor Author

@DiegoCardoso DiegoCardoso left a comment

Choose a reason for hiding this comment

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

FIxed review suggestions. I will check again if there's any left.

packages/dialog/src/vaadin-dialog.js Outdated Show resolved Hide resolved
packages/dialog/test/header-footer.test.js Outdated Show resolved Hide resolved
packages/dialog/src/vaadin-dialog.js Outdated Show resolved Hide resolved
packages/dialog/src/vaadin-dialog.js Outdated Show resolved Hide resolved
packages/dialog/src/vaadin-dialog.js Outdated Show resolved Hide resolved
packages/dialog/src/vaadin-dialog.js Show resolved Hide resolved
packages/dialog/src/vaadin-dialog.js Outdated Show resolved Hide resolved
@DiegoCardoso DiegoCardoso marked this pull request as ready for review April 1, 2022 11:15
@sonarcloud
Copy link

sonarcloud bot commented Apr 1, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

_headerTitleRenderer() {
if (this.headerTitle) {
if (!this.headerTitleElement) {
this.headerTitleElement = document.createElement('span');
Copy link
Contributor Author

@DiegoCardoso DiegoCardoso Apr 4, 2022

Choose a reason for hiding this comment

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

I was checking some component libraries and the ones I found use <h2> for the dialog title. Should we use it here, as well? @jouni @web-padawan

Examples:

Copy link
Member

Choose a reason for hiding this comment

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

We have a similar issue for vaadin-confirm-dialog header level (currently hardcoded to <h3>) and here is a comment that suggests not to hardcode it but let the developer choose: #467 (comment).

Copy link
Member

Choose a reason for hiding this comment

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

Let's create a follow-up issue and discuss this separately to not block this PR.

@vaadin-bot
Copy link
Collaborator

This ticket/PR has been released with Vaadin 23.1.0.alpha1 and is also targeting the upcoming stable 23.1.0 version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dialog header and footer
5 participants