seas-computing / mark-one

A UI component library for building React Apps (in development)
https://seas-computing.github.io/mark-one/
BSD 3-Clause "New" or "Revised" License
0 stars 1 forks source link

Add Mark One Props Interface #80

Closed natalynyu closed 3 years ago

natalynyu commented 3 years ago

This interface was added as a result of feedback from the course planner absence modal PR . In that ticket, I was using document.getElementById to get a reference to button elements to focus them. Jonathan Seitz recommended that we should be using ref instead. We had a discussion on this and concluded that since the properties ref and aria-label will eventually be used across many different mark one components, it would make sense to make an interface for these components at the top level of mark one so that component props could extend the interface to include these common props. We decided it made sense to only implement the interface for the components needed in the absence modal for now, which would be the button components.

Types of changes

Checklist:

Priority:

Related Issues:

Addresses #252

codecov[bot] commented 3 years ago

Codecov Report

Merging #80 (644e818) into develop (4fd23e8) will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop      #80   +/-   ##
========================================
  Coverage    98.24%   98.24%           
========================================
  Files           42       42           
  Lines          513      513           
  Branches        96       96           
========================================
  Hits           504      504           
  Misses           5        5           
  Partials         4        4           
Impacted Files Coverage Δ
src/Buttons/BorderlessButton.tsx 100.00% <ø> (ø)
src/Buttons/Button.tsx 100.00% <ø> (ø)
src/Theme/MarkOneWrapper.tsx 100.00% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 71423d4...644e818. Read the comment docs.

farassadek commented 3 years ago

Based on React docs (https://reactjs.org/docs/forwarding-refs.html) the on the Note for component library maintainers section When you start using forwardRef in a component library, you should treat it as a breaking change and release a new major version of your library. Therefore I think should be a braking change.