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.

Sign up for GitHub

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

Jump to bottom

feat(preview-middleware): introduce enhanced flp homepage #2967

Merged
merged 29 commits into from
Mar 20, 2025
Merged

feat(preview-middleware): introduce enhanced flp homepage #2967

merged 29 commits into from
Mar 20, 2025

Conversation

ui5red
Copy link
Contributor

@ui5red ui5red commented Feb 27, 2025 •
edited
Loading

  • Add new FLP homepage based on the sap.cux.home library controls
    • Toggle via boolean property flp.enhancedHomePage (false by default)
  • Update fallback UI5 version from 1.121.0 to 1.130.0
    • Ensure compatibility by loading original FLP homepage when using UI5 versions < 1.123.0 or for UI5 2.x until flp delivers a 2.0 compatible sandbox

Screenshot:
image

@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Feb 27, 2025 •
edited
Loading

🦋 Changeset detected

Latest commit: c52542a

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
@sap-ux/preview-middleware Minor
@sap-ux-private/preview-middleware-client Patch
@sap-ux/create Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@cla-assistant CLA Assistant
Copy link

cla-assistant bot commented Feb 27, 2025 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@heimwege
Copy link
Contributor

@ui5red maybe we need to increase the timeouts in case the integration tests keep failing in the changed setup. I re-triggered them. Let's see 🤞🏻

heimwege
heimwege reviewed Feb 27, 2025
View reviewed changes
Copy link
Contributor

@heimwege heimwege left a comment

Choose a reason for hiding this comment

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

See comments. Looks already very good 👍🏻
I just have some minor findings, some questions and some points that need clarification. Did not yet test just had a look at the code. Test will follow 🐱

@ui5red ui5red force-pushed the feat/preview-middleware/introduce-new-home branch 4 times, most recently from 42f6011 to c087064 Compare February 28, 2025 09:26
@ui5red ui5red force-pushed the feat/preview-middleware/introduce-new-home branch from 65e1c9b to e1e6d97 Compare February 28, 2025 14:19
@ui5red ui5red force-pushed the feat/preview-middleware/introduce-new-home branch 2 times, most recently from 982dc0a to 451cd74 Compare March 3, 2025 06:55
@ui5red ui5red force-pushed the feat/preview-middleware/introduce-new-home branch 3 times, most recently from 5137d41 to 5a52f84 Compare March 5, 2025 14:28
@ui5red ui5red requested a review from heimwege March 14, 2025 16:33
@heimwege
Copy link
Contributor

I just re-tested and saw that I get an error on the console from the Personalization Adapter
image
Do we need an additional resource root in the ui5 boostrap to overcome this?

ui5red added 2 commits March 17, 2025 17:54
@ui5red
Merge branch 'main' of github.com:SAP/open-ux-tools into feat/preview…
296b758 
…-middleware/introduce-new-home
@ui5red
fix: address review comments (part 5)
c4d18af 
- revert renderer selection logic
- include 'legacy-free' in disabling enhancedHomePage feature logic
- update README
@ui5red ui5red requested a review from heimwege March 17, 2025 12:34
@ui5red
Copy link
Contributor Author

ui5red commented Mar 18, 2025

I just re-tested and saw that I get an error on the console from the Personalization Adapter image Do we need an additional resource root in the ui5 boostrap to overcome this?

@heimwege fixed with 45fdb42 :)

heimwege
heimwege previously approved these changes Mar 19, 2025
View reviewed changes
Copy link
Contributor

@heimwege heimwege left a comment •
edited
Loading

Choose a reason for hiding this comment

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

  • test coverage is great
  • changeset exists
  • review comments addressed
  • changes are straightforward and new feature is nicely integrated into existing code
  • tested with:
    • UI5 1.132.x and enhancedHomePage = false -> no changes
    • UI5 1.132.x and enhancedHomePage = true -> new Home Page renders, app starts
    • UI5 2 SNAPSHOT and enhancedHomePage = false -> no changes
    • UI5 2 SNAPSHOT and enhancedHomePage = true -> log message that enhancedHomePage has been set to false
    • UI5 2 SNAPSHOT with enhancedHomePage = true and RTA -> no changes
    • UI5 1.132.x with enhancedHomePage = true and RTA -> no changes

@heimwege
Copy link
Contributor

@tobiasqueck do you think we need to mention that lib sap.cux.home is needed when serving via UI5 cli? So far we don't mention any lib, so I'm not sure if we want to start doing this in this PR or if we just need an additional PR to add all needed libs in the readme (own chapter or so).
...or we continue not mentioning them at all because it seems I'm the only one that does not use the fiori-tools-proxy 🙈

@tobiasqueck
Copy link
Contributor

tobiasqueck commented Mar 20, 2025 •
edited
Loading

@tobiasqueck do you think we need to mention that lib sap.cux.home is needed when serving via UI5 cli? So far we don't mention any lib, so I'm not sure if we want to start doing this in this PR or if we just need an additional PR to add all needed libs in the readme (own chapter or so). ...or we continue not mentioning them at all because it seems I'm the only one that does not use the fiori-tools-proxy 🙈

Sounds to me like a separate topic. We should have a different PR updating the readme's for all the users (you 😄 ) that don't use the fiori-tools-proxy.

tobiasqueck
tobiasqueck reviewed Mar 20, 2025
View reviewed changes
Copy link
Contributor

@tobiasqueck tobiasqueck left a comment

Choose a reason for hiding this comment

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

  • changeset ok
  • code is cleanly encapsulated and easy to understand
  • tests are ok

Problem: when testing locally, the news part looked out, it had the wrong size. I tested with UI5 1.133.0. @ui5red I sent you my internal test project to reproduce the issue

tobiasqueck
tobiasqueck previously approved these changes Mar 20, 2025
View reviewed changes
Copy link
Contributor

@tobiasqueck tobiasqueck left a comment

Choose a reason for hiding this comment

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

The issue found in testing is UI5 version specific and is gone with 1.134.0, therefore, I am happy to approve.

@ui5red ui5red dismissed stale reviews from tobiasqueck and heimwege via c52542a March 20, 2025 15:57
@ui5red ui5red force-pushed the feat/preview-middleware/introduce-new-home branch from 86e6534 to c52542a Compare March 20, 2025 15:57
@ui5red
Copy link
Contributor Author

ui5red commented Mar 20, 2025

  • changeset ok
  • code is cleanly encapsulated and easy to understand
  • tests are ok

Problem: when testing locally, the news part looked out, it had the wrong size. I tested with UI5 1.133.0. @ui5red I sent you my internal test project to reproduce the issue

issue fixed for versions below 1.134.0 and below with c52542a :)

heimwege
heimwege approved these changes Mar 20, 2025
View reviewed changes
Copy link
Contributor

@heimwege heimwege left a comment

Choose a reason for hiding this comment

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

re-approve

@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
96.1% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@ui5red ui5red merged commit 6e32009 into main Mar 20, 2025
16 checks passed
@ui5red ui5red deleted the feat/preview-middleware/introduce-new-home branch March 20, 2025 17:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tobiasqueck tobiasqueck tobiasqueck approved these changes

@heimwege heimwege heimwege approved these changes

Assignees
No one assigned
Labels
preview-middleware @sap-ux/preview-middleware preview-middleware-client
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ui5red @heimwege @tobiasqueck