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

Add the announcement component #2472

Closed
wants to merge 1 commit into from
Closed

Conversation

Tim-Siu
Copy link
Contributor

@Tim-Siu Tim-Siu commented Mar 24, 2024

What is the purpose of this pull request?

  • Documentation update
  • Bug fix
  • Feature addition or enhancement
  • Code maintenance
  • DevOps
  • Improve developer experience
  • Others, please explain:

Overview of changes:
Contribute to #2241
Related to #1960

This Pull request adds a announcement component. One usecase of the annoucement can be seen in the CS2103/T website.

Sample usage:

<announcement :dismissible="false" theme="warning" placement="bottom">
  <strong>This site is from a past semester!</strong> The current version will be <a href="http://www.comp.nus.edu.sg/~{{ module | lower }}">here</a> when the new semester starts.
</announcement>

<announcement :dismissible="true" theme="primary" placement="top-right">
  <strong>This site is from a past semester!</strong> The current version will be <a href="http://www.comp.nus.edu.sg/~{{ module | lower }}">here</a> when the new semester starts.
</announcement>

Output:

Screen.Recording.2024-03-24.at.17.30.12.mov

Parameters:

  1. dismissible: 'true', 'false;
  2. placement: 'top', 'top-right', 'top-left', 'bottom', 'bottom-right', 'bottom-left',
  3. theme: 'primary', 'secondary', 'success', 'warning', 'danger', 'info', 'light', 'dark',

Anything you'd like to highlight/discuss:
Discussions:
There are a lot of options we can add to this component.

Known limitations:

  1. Announcements may overlap each other
  2. Site wide application is not automatic

Testing instructions:

Proposed commit message: (wrap lines at 72 characters)
Add an announcement component


Checklist: ☑️

  • Updated the documentation for feature additions and enhancements
  • Added tests for bug fixes or features
  • Linked all related issues
  • No unrelated changes

Reviewer checklist:

Indicate the SEMVER impact of the PR:

  • Major (when you make incompatible API changes)
  • Minor (when you add functionality in a backward compatible manner)
  • Patch (when you make backward compatible bug fixes)

At the end of the review, please label the PR with the appropriate label: r.Major, r.Minor, r.Patch.

Breaking change release note preparation (if applicable):

  • To be included in the release note for any feature that is made obsolete/breaking

Give a brief explanation note about:

  • what was the old feature that was made obsolete
  • any replacement feature (if any), and
  • how the author should modify his website to migrate from the old feature to the replacement feature (if possible).

Copy link

codecov bot commented Mar 24, 2024

Codecov Report

Attention: Patch coverage is 0% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 50.97%. Comparing base (4ab7e3a) to head (975657f).

Files Patch % Lines
packages/vue-components/src/Announcement.vue 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2472      +/-   ##
==========================================
- Coverage   50.98%   50.97%   -0.02%     
==========================================
  Files         124      125       +1     
  Lines        5305     5307       +2     
  Branches     1137     1137              
==========================================
  Hits         2705     2705              
- Misses       2311     2313       +2     
  Partials      289      289              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@yucheng11122017
Copy link
Contributor

@damithc, could we get your opinion on whether component is needed?

@damithc
Copy link
Contributor

damithc commented Mar 24, 2024

@damithc, could we get your opinion on whether component is needed?

@Tim-Siu thanks for taking this on. @yucheng11122017 thx for checking.

I quite like the placement/behavior I have in https://nus-cs2103-ay2021s1.github.io/website/
The only thing missing is a way to dismiss it.
So, if there is a way to make such announcements easier to add and also dismissible, I think we can have it as a legit feature.

I prefer that to a toast that appears in front of other content and forces the user to dismiss it.

@Tim-Siu
Copy link
Contributor Author

Tim-Siu commented Mar 24, 2024

@damithc, could we get your opinion on whether component is needed?

@Tim-Siu thanks for taking this on. @yucheng11122017 thx for checking.

I quite like the placement/behavior I have in https://nus-cs2103-ay2021s1.github.io/website/ The only thing missing is a way to dismiss it. So, if there is a way to make such announcements easier to add and also dismissible, I think we can have it as a legit feature.

I prefer that to a toast that appears in front of other content and forces the user to dismiss it.

Sure. I will make the announcement component behave like that.

@LamJiuFong
Copy link
Contributor

I am thinking of adding a minimise button next to the dismiss button, the only difference is by clicking the minimise button, the user can always re-click it (somewhere) to make the announcement appear again.
Not sure if it is worth the effort implementing it.
What do you think?

dismiss() {
this.dismissed = true;
},
setPosition() {
Copy link
Contributor

@LamJiuFong LamJiuFong Mar 26, 2024

Choose a reason for hiding this comment

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

Since there are some repeating codes (width and height), would it be better to refactor setPosition() in this way:

// create a map
const placementStyles = {
  "top": { 
    top: '20px', 
    left: '50%', 
    transform: 'translateX(-50%)' 
  },
  "top-right": { 
    top: '20px', 
    right: '20px' 
  },
  "top-left": { 
    top: '20px', 
    left: '20px' 
  },
  "bottom": { 
    bottom: '20px', 
    left: '50%', 
    transform: 'translateX(-50%)' 
  },
  "bottom-right": { 
    bottom: '20px', 
    right: '20px' 
  },
  "bottom-left": { 
    bottom: '20px', 
    left: '20px' 
  }
}

setPosition() {
  const announcementElement = this.$refs.announcement;
  if (!announcementElement) return;
  const { width, height } = announcementElement.getBoundingClientRect();

  // get the respective styles from the map
  const positionStyle = { ...placementStyles[this.placement] };

  // since only width and height are specified in these cases
  if (this.placement.endsWith('right') || this.placement.endsWith('left')) {
    positionStyle.width = `${width}px`;
    positionStyle.height = `${height}px`;
  }

  this.positionStyle = positionStyle;
}

By doing this, next time if we introduce a new placement (eg. middle-left), we can just add the corresponding style to the map.

@yucheng11122017
Copy link
Contributor

yucheng11122017 commented Apr 4, 2024

Hi can you add documentation so its easier to test?

And also add test cases for this.
Please do look through the checkpoint on the PR description to see what you should do for each PR

@yucheng11122017 yucheng11122017 force-pushed the master branch 2 times, most recently from cb84513 to 69ec838 Compare April 5, 2024 06:21
@Tim-Siu Tim-Siu marked this pull request as draft April 6, 2024 02:18
@Tim-Siu
Copy link
Contributor Author

Tim-Siu commented Apr 8, 2024

After discussions with Prof. Demith, it appears that extending Box components to support announcements will be a better approach.

@Tim-Siu Tim-Siu closed this Apr 12, 2024
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.

4 participants