SAP / ui5-webcomponents-react

A wrapper implementation for React of the UI5 Web Components that are compliant with the SAP Fiori User Experience
https://sap.github.io/ui5-webcomponents-react/
Apache License 2.0
444 stars 100 forks source link

[Analytical Table]: Inconsistent Outside Border Styling #6477

Closed gitgdako closed 4 weeks ago

gitgdako commented 1 month ago

Describe the bug

According to the pictures in the Fiori doc https://experience.sap.com/fiori-design-web/analytical-table-alv/ tables shoud have a consistent border styling, which as can be seen in the storybook https://sap.github.io/ui5-webcomponents-react/v2/?path=/docs/data-display-analyticaltable--docs the implementation does not.

e.g. https://sap.github.io/ui5-webcomponents-react/v2/?path=/story/data-display-analyticaltable--default Header has starting border left, rows below do not Header has no top border, but the last row has a bottom border

This is obviously a very minor issue.

Isolated Example

No response

Reproduction steps

No response

Expected Behaviour

Consistent border styling: border everywhere around or nowhere

Screenshots or Videos

No response

UI5 Web Components for React Version

2.2.0

UI5 Web Components Version

2.3.0

Browser

Chrome

Operating System

Windows 11

Additional Context

No response

Relevant log output

No response

Organization

No response

Declaration

Lukas742 commented 1 month ago

Hi @gitgdako

thanks for reaching out! The docs you're referring to are still using the Fiori 3 (Quartz) theme family and probably weren't updated when specifications changed.

AnalyticalTable - sap_horizon:

image

AnalyticalTable - sap_fiori_3:

image
gitgdako commented 1 month ago

Hi. I am not quite sure what your point is. Horizon is as far as I understand a valid Fiori Color theme and the most up to date light theme. The screenshot youve posted is the same I am refering to with given issues

Header has starting border left, rows below do not Header has no top border, but the last row has a bottom border

The screenshot of Quartz light does not have the issue (the solution to my problem in fact) but we are not using this theme and according to the Fiori docs (https://experience.sap.com/fiori-design-web/quartz-light-colors/) this is an alternative theme, not the preferred one

Lukas742 commented 1 month ago

According to the pictures in the Fiori doc https://experience.sap.com/fiori-design-web/analytical-table-alv/

These screenshots are using Fiori 3, implementing different designs like border styles, colors, etc. Before Horizon was introduced, Fiori 3 (Quartz) was the preferred theme and therefore many screenshots on that site (https://experience.sap.com/fiori-design-web) still use the old theme.

gitgdako commented 1 month ago

Fair enough. But does the Horizon style expect the table to have this inconsistent border layout? I would expect it not to. Fundamental styles is another source having consistent border styling with discussion on different versions of the border styling with each having a consistent outer border https://sap.github.io/fundamental-styles/?path=/docs/sap-fiori-components-table--docs

Lukas742 commented 1 month ago

I'm not really sure what you mean with "inconsistent border layout".

Fundamental Styles seems to offer a way to configure borders, but this functionality doesn't seem to be covered by our specifications. You can see that for example in the SAPUI5 implementation as well, as there is also no way to configure border styles.

gitgdako commented 1 month ago

I dont expect it to be configurable, but I expect it to be consistent. Every outside border should be styled the same, either present or not. Your storybook as well as your screenshot shows that not all border have the same styling. E.g. in the Horizon screenshot the header has a different border on the left than the rows

Lukas742 commented 1 month ago

@gitgdako I think I get it now, thanks for clarifying. It's about this border here, right?

image

This is indeed not intended. Is there anything else that you find inconsistent?

gitgdako commented 1 month ago

Exactly. The same goes for the top and bottom border. When showing the "no data" placeholder it is not consistent as of now either. Again, it is not important what the solution looks like, but in all scenarios the border should be consistent, regardless of the configuration and data

Lukas742 commented 1 month ago

Just to avoid confusions, could you please create screenshots of each scenario that isn't consistent and mark the respective inconsistency? We'll then check if the style is implemented by design or not.

gitgdako commented 1 month ago

1) From https://sap.github.io/ui5-webcomponents-react/v2/?path=/story/data-display-analyticaltable--default

image

Red: Highlightrow left border and separator, both should be present imo Green: Top and bottom border, top should be present imo

2) From our application

image

Red: No Data Placeholder Row left and right border, should be present imo

I will update with further if I find any

gitgdako commented 1 month ago

I did not find any styleguide in the Fiori docs, but fundamental styles https://sap.github.io/fundamental-styles/?path=/docs/sap-fiori-components-table--docs in subsection "No outer Border" states the following "Table can be displayed without outer borders, might be needed when used inside some other element" I think in general having a border would look better unless having a different background within the table

OT: Does your storybook show your exact version of webcomponents/webcomponents-react?

Lukas742 commented 1 month ago

"Table can be displayed without outer borders, might be needed when used inside some other element" I think in general having a border would look better unless having a different background within the table

There are currently no global design specifications for this behavior. So, for this to be implemented, this request would need to go through the Inner Source process and be synchronised with the SAP Central Design team first. In case you're a SAP employee, please raise a BLI to the central design team and include the ticket number here for reference. Please contact me internally, if you require the current specifications or want to know the process of how to create BLIs for the central design team. If you don't have access to our internal systems, then please let me know. I'll then provide you with the necessary key points that need to be specified for the request.

OT: Does your storybook show your exact version of webcomponents/webcomponents-react?

For ui5-webcomponents-react, the versions can be derived from the URL

Since v2 you can also find our version in a window property. globalThis['@ui5/webcomponents-react'].Runtimes

For ui5-webcomponents the version is added to the generated meta tag inside the head (<meta name="ui5-shared-resources" content="">) --> E.g. console.log(document.querySelector('[name="ui5-shared-resources"]').Runtimes)

gitgdako commented 1 month ago

I am not a SAP employee, just a happy user of Fiori/UI5. Also I do not feel qualified to know how to best specify the guidelines for this UI styling. What I want it to be and to see this issue as resolved is simply consistent, so a recognizable pattern where a border is and where it isnt, it feels unintentional at the moment (and our users agree) Thanks for the info on the versions

Lukas742 commented 4 weeks ago

Hi @gitgdako the linked PR will fix the issue with the left most outer header border; by design no border should be present there.

It also changes/fixes:

Green: Top and bottom border, top should be present imo

Per specs, top table/header border should be omitted and bottom header border and bottom table border should have the same color.

Red: No Data Placeholder Row left and right border, should be present imo

This is by design.


Additionally, I want to emphasize that we truly appreciate your contributions. By creating issues and challenging the status quo, you help our library grow, so thank you!

ui5-webcomponents-react-bot commented 4 weeks ago

:tada: This issue has been resolved in version v2.3.0 :tada:

The release is available on v2.3.0

Your semantic-release bot :package::rocket:

gitgdako commented 4 weeks ago

Confirmed, styling consistent as intented