Skip to content

Conversation

RichDom2185
Copy link
Member

Description

Builds on #2706. Mainly an intermediate change (non-breaking), but simply done to:

  • Reduce code duplication
  • Standardise component interfaces

in order to lay the groundwork for eventual migration to a much better UI for searching using omnibox (as discussed with @sayomaki over Telegram).

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update
  • Code quality improvements

How to test

Everything should still work with regards to SICP search.

Checklist

  • I have tested this code
  • I have updated the documentation

@RichDom2185 RichDom2185 self-assigned this Oct 31, 2023
@RichDom2185 RichDom2185 changed the title Reduce code duplication in SICP search Reduce code duplication, standardise styling in SICP search Oct 31, 2023
@RichDom2185 RichDom2185 enabled auto-merge (squash) October 31, 2023 16:23
Copy link
Member Author

@RichDom2185 RichDom2185 left a comment

Choose a reason for hiding this comment

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

Let us aim to merge these two PRs quickly due to the importance of this feature for upcoming exam revision.

@RichDom2185 RichDom2185 requested a review from chownces October 31, 2023 16:42
@coveralls
Copy link

Pull Request Test Coverage Report for Build 6709669490

  • 1 of 13 (7.69%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.02%) to 37.191%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/commons/navigationBar/subcomponents/SicpNavigationBar.tsx 1 13 7.69%
Totals Coverage Status
Change from base Build 6709254880: 0.02%
Covered Lines: 5726
Relevant Lines: 14464

💛 - Coveralls

Copy link
Contributor

@sayomaki sayomaki left a comment

Choose a reason for hiding this comment

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

Good update to make use of the BlueprintJS components! However, the issue of content overflow is present, and I am still trying to see if there is a resolution for that.

As for the search buttons themselves, are they still of relevance as right now? Right now I believe it will automatically show your search results through the autocomplete when search input is provided.

@RichDom2185
Copy link
Member Author

Good update to make use of the BlueprintJS components! However, the issue of content overflow is present, and I am still trying to see if there is a resolution for that.

As for the search buttons themselves, are they still of relevance as right now? Right now I believe it will automatically show your search results through the autocomplete when search input is provided.

As discussed, this PR is simply a stepping stone to #2708. This PR only focuses on making the transition to Blueprint components (alongside some interface changes) to make the subsequent migration to use omnibox easier.

Regarding the two issues:

Copy link
Contributor

@sayomaki sayomaki left a comment

Choose a reason for hiding this comment

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

Alright, since this PR will be superseded by #2708 anyways, we can go ahead and merge it first. Everything seems to be working well except those in the aforementioned comment.

I will comment and add feedback on the more relevant changes over there instead. Thanks!

@RichDom2185 RichDom2185 merged commit f7ebbde into master Nov 1, 2023
@RichDom2185 RichDom2185 deleted the improve-sicp-ui-1 branch November 1, 2023 08:20
RichDom2185 added a commit to NUS-CS1101S/cadet-frontend that referenced this pull request Nov 4, 2023
…cademy#2707)

* Use BP Menu component for search results

* Simplify more menus

* Use BP MenuItem for IndexSearchResultsMenuEntry

* Improve typing

* Simplify index search results rendering

* Use BP MenuItem for UserSearchResultsMenuEntry

* Simplify text search results rendering

* Improve layout
RichDom2185 added a commit to NUS-CS1101S/cadet-frontend that referenced this pull request Nov 4, 2023
…cademy#2707)

* Use BP Menu component for search results

* Simplify more menus

* Use BP MenuItem for IndexSearchResultsMenuEntry

* Improve typing

* Simplify index search results rendering

* Use BP MenuItem for UserSearchResultsMenuEntry

* Simplify text search results rendering

* Improve layout
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.

3 participants