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(ObjectPage): Add 'Pin-Header' button #354

Merged
merged 38 commits into from
Mar 19, 2020

Conversation

MarcusNotheis
Copy link
Contributor

@MarcusNotheis MarcusNotheis commented Mar 17, 2020

This PR includes a full refactoring of the ObjectPage using native CSS capabilities instead of custom scroll handlers. This might lead to problems in IE11 and needs to be tested as well.

The Anchor Bar was replaced with the UI5 Web Component TabContainer in order to improve accessibility and RTL. --> #345

Closes #334
Closes #248

@MarcusNotheis MarcusNotheis requested review from vbersch, clemai and Lukas742 and removed request for vbersch March 17, 2020 08:02
@github-actions
Copy link

Coverage Status

Coverage decreased (-1.7%) to 66.74% when pulling 592a8dc on feat/object-page-sticky-header-button into 5297e76 on master.

@clemai
Copy link
Contributor

clemai commented Mar 18, 2020

Minor bugs:

  • Expand/Collapse button directs in the wrong direction when scrolling down:

image

  • 1.) Click to sub section -> OP scrolls to sub section 2.) Click to section of sub section -> OP doesn't scroll.

  • Little flickering of object page header when scrolling

@MarcusNotheis
Copy link
Contributor Author

The mentioned bugs should be now fixed - ready for re-review.
The header behaviour is now similar to the UI5 Object Page: When header is expanded in a scrolled down state, it will collapse on next scroll.

Copy link
Contributor

@vbersch vbersch left a comment

Choose a reason for hiding this comment

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

  • ObjectPage breaks when you remove selectedSectionId
  • ObjectPage should scroll to subsection if you pass 5.2 to selectedSubSectionId

@@ -17,6 +17,17 @@ export const Scroller = forwardRef((props: Props, ref: RefObject<IScroller>) =>

const elements = useRef([]);

useEffect(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

The Scroller is basically deprecated. No need to introduce breaking changes here.
Lets add a deprecation notice to the Scroller.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

contentScrollContainer: {
position: 'relative'
'&::-webkit-scrollbar': {
backgroundColor: '#ffffff',
Copy link
Contributor

Choose a reason for hiding this comment

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

Theming Parameters? Pretty sure we didn´t have them in place before, but shouldn´t we actually?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done


const anchorBarHeight = anchorBarRef.current?.offsetHeight ?? 33;

// const totalHeaderHeight =
Copy link
Contributor

Choose a reason for hiding this comment

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

still needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no :)

Copy link
Contributor

@clemai clemai left a comment

Choose a reason for hiding this comment

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

All my mentioned issues were addressed 👍

@MarcusNotheis MarcusNotheis merged commit 0e5e9b6 into master Mar 19, 2020
@MarcusNotheis MarcusNotheis deleted the feat/object-page-sticky-header-button branch March 19, 2020 08:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adding "Pin" Button to ObjectPage Component ObjectPage: Collection of known issues/limitations
3 participants