feat: introduce Quick View component #1898

EvilAlexei · 2020-11-19T08:43:58Z

Related Issue

No related issue

Description

This PR introduces the Quick View component

Please check whether the PR fulfills the following requirements

The output matches the design specs

The code follows fundamental-styles code standards and style

only one top level fd-* class is used in the file
BEM naming convention is used
Mixins are used for repeatable code (fd-rtl, fd-ellipsis, fd-flex, fd-selected, fd-focus, ect.)
A11y support - keyboard support, screenreader support, proper ARIA attributes, etc.
fd-reset() mixin is applied to all elements
Variables are used, if some value is used more than twice.
Checked if current components can be reused, instead of having new code.

Testing

tested Storybook examples with "CSS Resources" normalize option
tested Storybook examples with "CSS Resources" unnormalize option
Verified all styles in IE11
Updated tests

Documentation

Storybook documentation has been created/updated
Breaking Changes wiki has been updated in case of breaking changes.

netlify · 2020-11-19T08:47:10Z

✔️ Deploy preview for fundamental-styles ready!

🔨 Explore the source changes: bb2ac13

🔍 Inspect the deploy logs: https://app.netlify.com/sites/fundamental-styles/deploys/5feb28e0eda0f100079766ae

😎 Browse the preview: https://deploy-preview-1898--fundamental-styles.netlify.app

dimamarksman · 2020-12-14T09:17:34Z

Looks like an extra element:

dimamarksman · 2020-12-14T09:18:51Z

Two nested elements with the same selector:

dimamarksman · 2020-12-14T09:28:37Z

It would be great to add an example in RTL mode

Lokanathan-k · 2020-12-14T09:37:39Z

HI @EvilAlexei
content are not truncating instead expanding the popover.

Expected:

Actual implemented:

katekozlowska · 2020-12-14T10:15:52Z

stories/quick-view/quick-view.stories.js

+                </div>
+
+                <div class="fd-quick-view__group">
+                    <div class="fd-quick-view__group__title">


it should be -title instead of __title

EvilAlexei · 2020-12-14T12:04:41Z

Looks like an extra element:

This element is a content holder inside flex layout, also there is a fix for flex width :)

dimamarksman · 2020-12-14T14:08:11Z

Looks like an extra element:

This element is a content holder inside flex layout, also there is a fix for flex width :)

If it's just a container to follow a flex layout we should name it properly cause fd-quick-view__text does not describe the purpose it's used for (fd-quick-view__text looks like a common thing for any text element in scope of this component).

InnaAtanasova · 2020-12-14T16:13:45Z

Hi Oleksii,
from what I see in the specs Quick View is a composite component consisting of Header+Subheader+Content. Header and Subheader are extending the functionality of Bar component and the Content is extending the functionality of Form-Display Mode. This means that this component should contain classes that overwrite the classes of the mentioned components (if needed). For example if the header of the Quick view is different from the Bar header, then you create a modifier class fd-bar fd-bar--header fd-quick-view__header and fd-quick-view__header does not bring the reset. In the specs I didn't see any rule overwritten, which makes me think that this component should go into the "Patterns" category and be built out of already existing components.

stefanoScalzo · 2020-12-14T16:47:25Z

z index should be fixed

EvilAlexei · 2020-12-14T16:47:52Z

Looks like an extra element:

This element is a content holder inside flex layout, also there is a fix for flex width :)

If it's just a container to follow a flex layout we should name it properly cause fd-quick-view__text does not describe the purpose it's used for (fd-quick-view__text looks like a common thing for any text element in scope of this component).

It's a name based on semantic content, and it's a common name for such content.
Also, we already have similar class naming in a similar structure, in the card component: https://sap.github.io/fundamental-ngx/#/core/card#standard

In my opinion semantic name better than styling naming. It's higher chances that styles will be changed, but the type of content has fewer chances to be changed.

stefanoScalzo · 2020-12-14T16:48:14Z

we should have some type of overflow for the title

EvilAlexei · 2020-12-14T16:55:40Z

we should have some type of overflow for the title

It's based on the content used for the title. For example, if we use fd-title

EvilAlexei · 2020-12-14T17:02:25Z

z index should be fixed

This is a popover issue, not related to the quick view styles.

dimamarksman · 2020-12-15T09:09:52Z

Looks like an extra element:

This element is a content holder inside flex layout, also there is a fix for flex width :)

If it's just a container to follow a flex layout we should name it properly cause fd-quick-view__text does not describe the purpose it's used for (fd-quick-view__text looks like a common thing for any text element in scope of this component).

It's a name based on semantic content, and it's a common name for such content.
Also, we already have similar class naming in a similar structure, in the card component: https://sap.github.io/fundamental-ngx/#/core/card#standard

In my opinion semantic name better than styling naming. It's higher chances that styles will be changed, but the type of content has fewer chances to be changed.

From provided card example the class name is fd-card__header-text that means text area in header scope. But here you use fd-quick-view__text that is more general thing. Let's use fd-quick-view__header-text or something similar to point the scope this text is belong to.

JKMarkowski

Hello @EvilAlexei I added some comments, that will drastically change component structure. This should be more composition of components, not standalone component. I can't see any specification for quick view. PM me, if you willl have any questions

src/quick-view.scss

JKMarkowski · 2020-12-15T10:27:38Z

src/quick-view.scss

+  min-width: 20rem;
+  max-width: 30rem;


Where are those limits from?

I wasn't able to find any info about those limits in the design docs,
but this component has already pre-defined content and I think those limits are necessary for the view perception.

For min-width I used a width value from [examples], (https://sapui5.hana.ondemand.com/#/entity/sap.m.QuickView/sample/sap.m.sample.QuickView) They all have 320px width.

I wasn't able to use max-width as well from those examples because it is dynamic there,
so 30rem is my assumption

Could you then put it as a hardcoded value into examples? You can use inline-styling or check how it's done with dialog

src/quick-view.scss

JKMarkowski

Hello @EvilAlexei Great job with refactoring this component. You are almost there, I added some code related comments

JKMarkowski · 2020-12-22T09:15:39Z

src/quick-view.scss

+  min-width: 20rem;
+  max-width: 30rem;


Could you then put it as a hardcoded value into examples? You can use inline-styling or check how it's done with dialog

src/quick-view.scss

JKMarkowski · 2020-12-22T09:23:35Z

stories/quick-view/quick-view.stories.js

+</div>
+`;
+
+RTLMode.storyName = 'RTL mode';


Let's also remove RTL mode example, there is a way to control direction on docs header

EvilAlexei · 2020-12-22T10:13:31Z

Hello @JKMarkowski, thank you for your comments!

About min-width and max-width, just I think it's better to have these limitations for a better view,
because of pre-defined content and structure.

About min-width:
For example, it's ok for the popover to have no such limitations, It could be used with any sort of content,
but the dialog has pre-defined min-width because you have an assumption about content there.

About max-width:
I see two almost the same ways setting width here, to set a specific width, or to have min/max-width.
With the specific width, a user has to set it all the time.
But with such limitations, it could be more dynamic, and a user still able to set width, but it's not necessary.

stefanoScalzo · 2020-12-22T16:58:23Z

we should have some type of overflow for the title

It's based on the content used for the title. For example, if we use fd-title

Then maybe we should use fd-title, there should be some type of ellipses standard for text overflow

EvilAlexei · 2020-12-22T17:05:11Z

we should have some type of overflow for the title

It's based on the content used for the title. For example, if we use fd-title

Then maybe we should use fd-title, there should be some type of ellipses standard for text overflow

Yeap, I already used it with fd-title, and fd-link with title needs additional adjustments, I added styles for that, now it should work as requested.

JKMarkowski · 2020-12-28T08:25:56Z

About min-width and max-width, just I think it's better to have these limitations for a better view,
because of pre-defined content and structure.

About min-width:
For example, it's ok for the popover to have no such limitations, It could be used with any sort of content,
but the dialog has pre-defined min-width because you have an assumption about content there.

About max-width:
I see two almost the same ways setting width here, to set a specific width, or to have min/max-width.
With the specific width, a user has to set it all the time.
But with such limitations, it could be more dynamic, and a user still able to set width, but it's not necessary.

Hello @EvilAlexei, thanks for complex response, I totally agree with you that those limitations will be a little handy for users, but it's same situation as with dialogs, or card. We provide such a limits on docs level, because specification doesn't specify them. Let's keep it same with this component.

Everything else looks great for me. Good job!

JKMarkowski

LGTM

katekozlowska

Looks good!

InnaAtanasova changed the title ~~WIP feat: (quick-view) introduce quick view component~~ [WIP] feat: introduce Quick View component Nov 25, 2020

This was referenced Dec 4, 2020

Sprint 51 - goals SAP/fundamental-ngx#3389

Closed

Sprint 52 - goals SAP/fundamental-ngx#3732

Closed

InnaAtanasova requested a review from a team December 8, 2020 15:28

InnaAtanasova assigned EvilAlexei Dec 8, 2020

InnaAtanasova added the Fiori New Component label Dec 8, 2020

artolshansky mentioned this pull request Dec 10, 2020

feat: (Core) introduce Quick View component SAP/fundamental-ngx#4077

Closed

4 tasks

EvilAlexei changed the title ~~[WIP] feat: introduce Quick View component~~ feat: introduce Quick View component Dec 14, 2020

katekozlowska suggested changes Dec 14, 2020

View reviewed changes

EvilAlexei requested review from katekozlowska, dimamarksman and InnaAtanasova December 14, 2020 12:05

dimamarksman approved these changes Dec 15, 2020

View reviewed changes

JKMarkowski suggested changes Dec 15, 2020

View reviewed changes

JKMarkowski changed the base branch from master to main December 17, 2020 10:53

feat: (quick-view) introduce quick view component

1de290f

Oleksii Aleksandrov added 3 commits December 18, 2020 17:20

feat: (quick-view) added new examples along with style changes

3ba097d

fix: (quick-view) added changes accroding to the review

3bb73cc

fix: (quick-view) changed subheader text class after PR review

1c0422c

EvilAlexei requested a review from JKMarkowski December 20, 2020 11:22

fix: (Quick View) added changes according to review

2d90cfc

JKMarkowski suggested changes Dec 22, 2020

View reviewed changes

fix: (quick view) added fix accroding PR review

db152e1

EvilAlexei requested a review from JKMarkowski December 22, 2020 10:28

EvilAlexei mentioned this pull request Dec 24, 2020

Quick View - styles SAP/fundamental-ngx#4075

Closed

fix: (quick view) removed max-width according to PR review

bb2ac13

JKMarkowski approved these changes Dec 29, 2020

View reviewed changes

katekozlowska approved these changes Dec 29, 2020

View reviewed changes

JKMarkowski merged commit 04249c2 into SAP:main Dec 29, 2020

JKMarkowski mentioned this pull request Feb 19, 2021

Integrating the latest version of Fundamental-styles in Fundamental-ngx SAP/fundamental-ngx#3684

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: introduce Quick View component #1898

feat: introduce Quick View component #1898

EvilAlexei commented Nov 19, 2020

netlify bot commented Nov 19, 2020 •

edited

dimamarksman commented Dec 14, 2020

dimamarksman commented Dec 14, 2020

dimamarksman commented Dec 14, 2020

Lokanathan-k commented Dec 14, 2020

katekozlowska Dec 14, 2020

EvilAlexei commented Dec 14, 2020 •

edited by dimamarksman

dimamarksman commented Dec 14, 2020 •

edited

InnaAtanasova commented Dec 14, 2020

stefanoScalzo commented Dec 14, 2020

EvilAlexei commented Dec 14, 2020

stefanoScalzo commented Dec 14, 2020

EvilAlexei commented Dec 14, 2020 •

edited

EvilAlexei commented Dec 14, 2020

dimamarksman commented Dec 15, 2020 •

edited

JKMarkowski left a comment •

edited

JKMarkowski Dec 15, 2020

EvilAlexei Dec 21, 2020

JKMarkowski Dec 22, 2020

JKMarkowski left a comment

JKMarkowski Dec 22, 2020

JKMarkowski Dec 22, 2020

EvilAlexei Dec 22, 2020

EvilAlexei commented Dec 22, 2020

stefanoScalzo commented Dec 22, 2020

EvilAlexei commented Dec 22, 2020

JKMarkowski commented Dec 28, 2020

JKMarkowski left a comment

katekozlowska left a comment

feat: introduce Quick View component #1898

feat: introduce Quick View component #1898

Conversation

EvilAlexei commented Nov 19, 2020

Related Issue

Description

Please check whether the PR fulfills the following requirements

netlify bot commented Nov 19, 2020 • edited

dimamarksman commented Dec 14, 2020

dimamarksman commented Dec 14, 2020

dimamarksman commented Dec 14, 2020

Lokanathan-k commented Dec 14, 2020

katekozlowska Dec 14, 2020

Choose a reason for hiding this comment

EvilAlexei commented Dec 14, 2020 • edited by dimamarksman

dimamarksman commented Dec 14, 2020 • edited

InnaAtanasova commented Dec 14, 2020

stefanoScalzo commented Dec 14, 2020

EvilAlexei commented Dec 14, 2020

stefanoScalzo commented Dec 14, 2020

EvilAlexei commented Dec 14, 2020 • edited

EvilAlexei commented Dec 14, 2020

dimamarksman commented Dec 15, 2020 • edited

JKMarkowski left a comment • edited

Choose a reason for hiding this comment

JKMarkowski Dec 15, 2020

Choose a reason for hiding this comment

EvilAlexei Dec 21, 2020

Choose a reason for hiding this comment

JKMarkowski Dec 22, 2020

Choose a reason for hiding this comment

JKMarkowski left a comment

Choose a reason for hiding this comment

JKMarkowski Dec 22, 2020

Choose a reason for hiding this comment

JKMarkowski Dec 22, 2020

Choose a reason for hiding this comment

EvilAlexei Dec 22, 2020

Choose a reason for hiding this comment

EvilAlexei commented Dec 22, 2020

stefanoScalzo commented Dec 22, 2020

EvilAlexei commented Dec 22, 2020

JKMarkowski commented Dec 28, 2020

JKMarkowski left a comment

Choose a reason for hiding this comment

katekozlowska left a comment

Choose a reason for hiding this comment

netlify bot commented Nov 19, 2020 •

edited

EvilAlexei commented Dec 14, 2020 •

edited by dimamarksman

dimamarksman commented Dec 14, 2020 •

edited

EvilAlexei commented Dec 14, 2020 •

edited

dimamarksman commented Dec 15, 2020 •

edited

JKMarkowski left a comment •

edited